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

[rorueda]

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]

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.

[rubenada]

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)

[rorueda]

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.

[rubenada]

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?

[rorueda]

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.

[rubenada]

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?

[rorueda]

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

[rubenada]

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);

[rubenada]

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)

[rorueda]

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]

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]

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]

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]

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

[rubenada]

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]

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'.

[sonarcloud]

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