Conversation
1f2c71e to
fcf2e41
Compare
Custom hash and merge join comparisons keep a raw function anchor while Plan conversion regenerates all declarations. A comparison referencing equal:any_any at anchor 1 can therefore resolve to not_equal:any_any after a round trip; a comparison-only function loses its declaration entirely. Store the resolved scalar function declaration in CustomComparison, resolve it against the input plan's lookup, and register it with the output collector. This preserves identity across anchor reassignment and works with custom extension collections. BREAKING CHANGE: CustomComparison.of(int), getCustomFunctionReference(), and the generated customFunctionReference(int) builder method are replaced by of(ScalarFunctionVariant), getDeclaration(), and declaration(ScalarFunctionVariant). Supply the comparator declaration from your extension collection instead of a plan-local integer anchor.
fcf2e41 to
cdf5187
Compare
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Please extend the BREAKING CHANGE: footer to cover the read-path break too, since that's the only part of the body that reaches the release notes. Reading a plan now throws unless every custom comparison's function is declared and its URN is loaded, and earlier substrait-java releases never wrote that declaration, so their plans with custom comparisons no longer read. Something like:
BREAKING CHANGE: CustomComparison.of(int), getCustomFunctionReference(), and the generated customFunctionReference(int) builder method and withCustomFunctionReference(int) wither are replaced by of(ScalarFunctionVariant), getDeclaration(), declaration(ScalarFunctionVariant) and withDeclaration(ScalarFunctionVariant). Supply the comparator declaration from your extension collection instead of a plan-local integer anchor. Reading a plan now requires each custom comparison function to be declared in the plan's extensions, with its URN loaded in the ExtensionCollection; plans with custom comparisons written by earlier substrait-java releases lack that declaration and must have one added before they can be read.
| return ImmutableComparisonJoinKey.CustomComparison.builder() | ||
| .customFunctionReference(customFunctionReference) | ||
| .build(); | ||
| public static CustomComparison of(SimpleExtension.ScalarFunctionVariant declaration) { |
There was a problem hiding this comment.
Should CustomComparison get a @Value.Check that the declaration takes two arguments and returns boolean, as the spec requires? Right now of(add:i32_i32) or of(not:bool) round-trips without error; the check is cheap now that the declaration is held, and adding it in this already-breaking PR avoids a second break later.
| * A custom comparison behavior, given by a reference to a binary function with a boolean return | ||
| * type. | ||
| */ | ||
| /** A custom comparison behavior, given by a binary scalar function with a boolean return type. */ |
There was a problem hiding this comment.
The spec only says "a binary function with a boolean return type" and doesn't restrict the function kind, so resolving it as a scalar function is substrait-java's choice. Could you say so here (and in the PR body), e.g. "substrait-java resolves it as a scalar function"?
| SimpleExtension.FunctionAnchor.of( | ||
| DefaultExtensionCatalog.FUNCTIONS_COMPARISON, "equal:any_any")); | ||
|
|
||
| private static Stream<Arguments> joinCases() { |
There was a problem hiding this comment.
Consider replacing this 16-case nested flatMap with a flat @CsvSource of 4–5 rows, e.g. (hash, 1, filter), (merge, 1, filter), (hash, 0, no filter), (merge, -1, no filter). Those catch the same regressions and give readable case names. The comment on line 48 also reads as wire-level coverage, but the test never serializes.
| } | ||
| Plan.Builder plan = | ||
| Plan.newBuilder() | ||
| .setVersion(Version.newBuilder().setMinorNumber(102)) |
There was a problem hiding this comment.
Drop this setVersion(...) line and the Version import: nothing reads the plan version, and the hard-coded 102 will go stale on the next spec bump.
| merge ? rel.getMergeJoin().getKeysList() : rel.getHashJoin().getKeysList(); | ||
| assertEquals(2, keys.size()); | ||
| int reference = keys.get(0).getComparison().getCustomFunctionReference(); | ||
| assertEquals(reference, keys.get(1).getComparison().getCustomFunctionReference()); |
There was a problem hiding this comment.
Nit, optional: the extension-count and round-trip assertions already catch a divergent second reference, so this line can go.
| assertEquals(reference, keys.get(1).getComparison().getCustomFunctionReference()); |
2 participants
Custom hash and merge join comparisons keep a raw function anchor while Plan conversion regenerates all declarations. A comparison referencing equal:any_any at anchor 1 can therefore resolve to not_equal:any_any after a round trip; a comparison-only function loses its declaration entirely.
Store the resolved scalar function declaration in CustomComparison, resolve it against the input plan's lookup, and register it with the output collector. This preserves identity across anchor reassignment and works with custom extension collections.
BREAKING CHANGE: CustomComparison.of(int), getCustomFunctionReference(), and the generated customFunctionReference(int) builder method are replaced by of(ScalarFunctionVariant), getDeclaration(), and declaration(ScalarFunctionVariant). Supply the comparator declaration from your extension collection instead of a plan-local integer anchor.