Remove deprecated FunctionRegistry methods and inline CalciteSqlParser OPTIONS helpers#18789
Conversation
…r OPTIONS helpers - Drop FunctionRegistry.containsFunction(String) and getFunctionInfo(String, int) — both had zero callers outside the class and were already marked @deprecated. Callers should use contains(canonicalName) and lookupFunctionInfo(canonicalName, n). - Inline the three private @deprecated helpers in CalciteSqlParser (extractOptionsFromSql, removeOptionsFromSql, extractOptionsMap) directly into compileToSqlNodeAndOptions and delete the now-dead methods. Behavior is identical; the only change is eliminating the indirection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Akanksha-kedia
force-pushed
the
remove-deprecated-function-registry-methods
branch
from
8a58d61 to
ef5c6ba
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18789 +/- ##
============================================
- Coverage 64.78% 64.78% -0.01%
Complexity 1309 1309
============================================
Files 3380 3381 +1
Lines 209540 209948 +408
Branches 32797 32887 +90
============================================
+ Hits 135751 136013 +262
- Misses 62863 62977 +114
- Partials 10926 10958 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal compatibility issue; see inline comment.
| * {@link #canonicalize(String)} multiple times. | ||
| */ | ||
| @Deprecated | ||
| public static boolean containsFunction(String name) { |
There was a problem hiding this comment.
Removing the deprecated containsFunction(String) / getFunctionInfo(String,int) wrappers is still a binary-incompatible change for callers compiled against released pinot-common jars. Upgraders that still invoke those entry points will start failing with NoSuchMethodError, so Pinot usually needs to keep the old methods as delegating shims until there is an explicit compatibility-removal plan.
There was a problem hiding this comment.
Good point, thank you. I'll restore containsFunction(String) and getFunctionInfo(String,int) as delegating shims so callers compiled against released jars don't get NoSuchMethodError. The CalciteSqlParser inlining (private methods only) should remain unaffected — will update this PR shortly.
There was a problem hiding this comment.
Done — containsFunction(String) and getFunctionInfo(String, int) restored as delegating shims in the latest commit. PR now only contains the CalciteSqlParser inlining (private methods, no binary-compat impact).
|
@xiangfu0 @Jackie-Jiang all CI checks pass, review comments addressed. Please review when you get a chance. |
Per xiangfu0's review: containsFunction(String) and getFunctionInfo(String, int) are public methods on a released pinot-common artifact; removing them would cause NoSuchMethodError for callers compiled against older jars. Restore as delegating stubs pending an explicit removal plan.
3 participants
Summary
FunctionRegistry.containsFunction(String)andgetFunctionInfo(String, int)— both were marked@Deprecatedwith no callers outside the class@Deprecatedhelpers inCalciteSqlParser(extractOptionsFromSql,removeOptionsFromSql,extractOptionsMap) directly intocompileToSqlNodeAndOptionsand delete the dead methods; behaviour is unchangedTest plan
CalciteSqlParserTestandFunctionRegistryTestpass — no API surface was removed that had external callers