[CALCITE-6593] NPE when outer joining tables with many fields and unmatching rows #3977

rorueda · 2024-09-24T12:42:42Z

The fix is quite simple. I added tests for left join and right join with empty tables.

The test file changed a bit more than expected because:

I extracted some code to be reused in the other tests;
I removed a query.WithHook because it was not being used, as this call doesn't mutate the query object;
I changed the field names to be able to select them easily on the right outer join.

rubenada

LGTM

rubenada · 2024-09-24T15:09:49Z

Thanks for the patch @rorueda , it looks good to me.
If you have a bit of time, could you please squash commits into a single one? (please use the new, more precise title of the Jira/PR in the final commit message).
I plan to merge this in the next 24/4 h if no objection appears.

core/src/main/java/org/apache/calcite/adapter/enumerable/JavaRowFormat.java

rubenada · 2024-09-25T09:58:05Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java

-      compactCode.add(exp);
-    } else {
-      compactOutputVar = null;
+    if (outputFieldCount >= 100) {


Have you checked this patch with a lower value of outputFieldCount?
What would you think about committing temporarily in the branch a lower threshold here (e.g. 1 or 2) and see how the CI checks behave? (and of course reverting this temp change before merging)

I think it's a good idea.

I also don't like this hardcoded there. Is there a better, centralized, place to put this? Then we could also generate the test tables using it, instead of having to hardcode it again in the test class.

Maybe we could define a new CalciteSystemProperty of type Integer, perhaps with defaultValue 100, but if the users sets it to a negative value then the compactCode version is disable? (just to have a way to "disable" this feature and fallback to the original code in case a problem is found with the compactCodeGenerator afterwards). WDYT?

I like the idea. Then it becomes easier also to run all the tests with a specific value, or to benchmark a real use case with different values. If nobody opposes, I can do it.

Thanks @rorueda . The CI checks with the temporary (outputFieldCount > 0) threshold have passed successfully, so the new compact code seems stable 👏
I guess the only remaining bit is reverting this temp change, setting the threshold via a new CalciteSystemProperty of type int, with defaultValue 100 and if <0 then codeCompact is disable. WDYT?

I think I will be able to do this at the end of the day. Any suggestion for the property name?

I'd propose something along the lines:

/** * Minimum numbers of fields in a Join result that will trigger the "compact code generation". * This feature reduces the risk of running into a compilation error due to the code of a * dynamically generated method growing beyond the 64KB limit. * * <p>The default value is 100, a negative value disables completely the "compact code" feature. * * @see org.apache.calcite.adapter.enumerable.EnumUtils */ public static final CalciteSystemProperty<Integer> JOIN_SELECTOR_COMPACT_CODE_THRESHOLD = intProperty("calcite.join.selector.compact.code.threshold", 100);

Also, please verify that setting the new CalciteSystemProperty to e.g. -1 causes all test to pass, except for LargeGeneratedJoinTest (which should fail since it should reach the 64KB error if compact code is not used)

I used your code to add the property, only adding a note on performance. Also had to modify the test to fail when the property is set to -1.

To run tests disabling compact code generation:
./gradlew -Dcalcite.join.selector.compact.code.threshold=-1 test

To run tests enabling compact code generation for any join size:
./gradlew -Dcalcite.join.selector.compact.code.threshold=0 test

rorueda · 2024-09-25T10:40:52Z

Summarizing the changes after more issues were discovered (check jira for details):

EnumUtils:

Split the code paths of compact code and normal code. It sacrifices a bit of code reuse, but it makes it clearer and minimizes the chance of adding bugs to the normal code path.
On the compact code path, remove the "optimization" of generating an array of a specific type if all output fields are of the same type - I don't think there is any benefit in doing this and the normal code path also doesn't seem to do this.

JavaRowType:

Make the copy method abstract (the default implementation, disregarding the different types, was the big issue).
Implement a specific copy for every JavaRowType.

rubenada · 2024-09-25T14:09:40Z

Summarizing the changes...

Just to complete your summary, I think you missed in your comment (but you implemented in your patch) the fact that in EnumUtils the compact code was missing the early break in case of semi/anti joins (i.e. when the output type contains only the types of the first input)

rubenada · 2024-09-26T08:30:37Z

Patch looks good. Thanks @rorueda

@julianhyde , why did you force push here? It seems the PR now contains several changes unrelated to the ticket.

rorueda · 2024-09-26T11:46:50Z

I rebased the changes on top of the current master to make it easier to review.

rubenada · 2024-09-26T11:55:22Z

Thanks @rorueda , I just saw that Julian had to force-push on main to correct one commit.
Now the PR is clearer.

As I said before, I think the PR is in a good shape, it only remains to squash commits into a single one (you can do that whenever you want). I plan to merge this in the next 24/48h if no further objection appears.

julianhyde · 2024-09-26T19:42:10Z

I force-pushed to change Mihai's commit message from 'RexBuilder can't handle NaN,Infinity double constants' to 'RexBuilder can't handle NaN, Infinity double constants'.

…atching rows EnumUtils: - Split the code paths of compact code and normal code; - Keep the normal code path as is and change the compact code path: - Generate a null check if the row might be null; - Add the early break in case of semi/anti joins; - Remove the "optimization" of generating an array of a specific type if all output fields are of the same type; JavaRowType: - Make the copy method abstract and implement a specific copy for every JavaRowType; Also, add a new system property to determine the threshold that triggers compact code generation.

sonarcloud · 2024-09-27T14:56:31Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
72.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

rorueda changed the title ~~[CALCITE-6593] NPE when joining tables with many fields and one table is empty~~ [CALCITE-6593] NPE when outer joining tables with many fields and unmatching rows Sep 24, 2024

rubenada approved these changes Sep 24, 2024

View reviewed changes

rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 24, 2024

rubenada removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 24, 2024

rorueda force-pushed the fix-npe-join branch from 50d7af7 to 3862a10 Compare September 24, 2024 15:42

rubenada added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Sep 24, 2024

rorueda force-pushed the fix-npe-join branch from 3862a10 to 3c4e41c Compare September 25, 2024 09:52

rubenada reviewed Sep 25, 2024

View reviewed changes

core/src/main/java/org/apache/calcite/adapter/enumerable/JavaRowFormat.java Outdated Show resolved Hide resolved

rubenada reviewed Sep 25, 2024

View reviewed changes

julianhyde force-pushed the main branch from 8d3cb82 to 4ab8d80 Compare September 25, 2024 18:04

rubenada added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR labels Sep 26, 2024

rorueda force-pushed the fix-npe-join branch from 714a7ef to d67af52 Compare September 26, 2024 11:44

rorueda force-pushed the fix-npe-join branch from d67af52 to 85da1a3 Compare September 27, 2024 14:36

rubenada merged commit a8802c7 into apache:main Sep 27, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6593] NPE when outer joining tables with many fields and unmatching rows #3977

[CALCITE-6593] NPE when outer joining tables with many fields and unmatching rows #3977

rorueda commented Sep 24, 2024

rubenada left a comment

rubenada commented Sep 24, 2024

rubenada Sep 25, 2024

rorueda Sep 25, 2024

rubenada Sep 25, 2024 •

edited

Loading

rorueda Sep 25, 2024

rubenada Sep 25, 2024

rorueda Sep 25, 2024

rubenada Sep 25, 2024

rubenada Sep 25, 2024

rorueda Sep 25, 2024

rorueda commented Sep 25, 2024

rubenada commented Sep 25, 2024 •

edited

Loading

rubenada commented Sep 26, 2024

rorueda commented Sep 26, 2024

rubenada commented Sep 26, 2024

julianhyde commented Sep 26, 2024

sonarcloud bot commented Sep 27, 2024

[CALCITE-6593] NPE when outer joining tables with many fields and unmatching rows #3977

[CALCITE-6593] NPE when outer joining tables with many fields and unmatching rows #3977

Conversation

rorueda commented Sep 24, 2024

rubenada left a comment

Choose a reason for hiding this comment

rubenada commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenada Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rorueda commented Sep 25, 2024

rubenada commented Sep 25, 2024 • edited Loading

rubenada commented Sep 26, 2024

rorueda commented Sep 26, 2024

rubenada commented Sep 26, 2024

julianhyde commented Sep 26, 2024

sonarcloud bot commented Sep 27, 2024

Quality Gate passed

rubenada Sep 25, 2024 •

edited

Loading

rubenada commented Sep 25, 2024 •

edited

Loading