Skip to content

[CALCITE-5948] Use explicit casting if element type in ARRAY/MAP does not equal derived component type - #3395

Merged
tanclary merged 1 commit into
apache:mainfrom
taoran92:CALCITE-5948
Oct 9, 2023
Merged

tanclary merged 1 commit into
apache:mainfrom
taoran92:CALCITE-5948

Conversation

@taoran92

Copy link
Copy Markdown
Member

@taoran92
taoran92 force-pushed the CALCITE-5948 branch 4 times, most recently from 1556351 to 8075f6c Compare August 29, 2023 03:37
} else {
elementType = oddType;
}
if (operandTypes.get(i).equalsSansFieldNames(elementType)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be rewritten as if(!operandTypes.get(i)....) { call.setOperand() }

@taoran92 taoran92 Sep 1, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good point. fixed. thanks tanner for reviewing.

Comment thread core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java Outdated
@taoran92

taoran92 commented Sep 4, 2023

Copy link
Copy Markdown
Member Author

@tanclary hi, tanner. I have solved some comments. If you have time, could you help to review it again? thanks!

@taoran92

taoran92 commented Sep 6, 2023 •

Copy link
Copy Markdown
Member Author

@tanclary hi, Tanner. Sorry to ping you. It's been two weeks, could you help to review it again? Looking forward your comments.

+------------------+
| EXPR$0 |
+------------------+
| [a , null, bcd] |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there an extra space?

@taoran92 taoran92 Sep 18, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JiajunBernoulli thanks for reviewing. this is a import change in this PR.

if we have array('A', 'AB'), the derived component type is char(2), the old behavior not cast 'A' to char(2) in runtime to match derived component type. The correct behavior let 'A' match the char(2), 'A' cast to char(2), then it will fill with extra spaces by default in calcite.

This is calcite and sql standard limitation. Of course, this does look weird, actually calcite has other ways to remove spaces. We can activate SqlConformanceEnum.PRAGMATIC_2003 to let shouldConvertRaggedUnionTypesToVarying be true, it will change array/map char type to varchar type to skip these spaces.

More details can be found in javadoc https://github.com/apache/calcite/blob/d9dd3ac8a9f695e111a0a5e77f45b61b90f4b5b6/core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java#L465C7-L465C7

I also have added some cases in the PR to show this difference.

@taoran92 taoran92 Sep 19, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JiajunBernoulli hi, Jiajun. If you have time, could you help to review it again? thanks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Thank you for your clear explanation.

@tanclary tanclary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chucheng92 Sorry for delay, thanks for addressing the changes. I'm okay to merge this as soon as @JiajunBernoulli is too.

@taoran92 taoran92 changed the title [CALCITE-5948] Explicit casting should be made if the type of an element in ARRAY/MAP not equals with the derived component type [CALCITE-5948] Explicit casting should be made if the type of an element in ARRAY/MAP not equals the derived component type Sep 20, 2023
@taoran92

Copy link
Copy Markdown
Member Author

Thanks Tanner and Jiajun for reviewing. All comments are resolved/confirmed.
I have rebased main and squashed the commits and made commit name to match Jira name.
hi @tanclary If you have time, could you help to merge this?

@taoran92
taoran92 requested a review from tanclary September 20, 2023 16:02
@taoran92

taoran92 commented Sep 21, 2023 •

Copy link
Copy Markdown
Member Author

@julianhyde hi, julian. I noticed that you gave a comment in the ticket to suggest giving a test case for calcite-5960. I have added it, if you have time, could you help to review it? If you approve that, then i will squash the commits. thanks!

Comment thread core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java Outdated
@taoran92
taoran92 force-pushed the CALCITE-5948 branch 2 times, most recently from 8704704 to 4feaa6b Compare September 21, 2023 08:41
@taoran92

Copy link
Copy Markdown
Member Author

@julianhyde hi, julian. I noticed that you gave a comment in the ticket to suggest giving a test case for calcite-5960. I have added it, if you have time, could you help to review it? If you approve that, then i will squash the commits. thanks!

calcite-5960 has been resolved.

@taoran92

taoran92 commented Sep 21, 2023 •

Copy link
Copy Markdown
Member Author

thanks Julian and Vladimir for reviewing the related commit calcite-5960. Considering that commit has been resolved, so I have rebased this PR. Currently all comments are resolved.

Hi, @tanclary sorry to ping you, if you have time, could you help to recheck/merge this?
thank you all again for patient reviewing.

Comment thread babel/src/test/resources/sql/big-query.iq
@taoran92 taoran92 changed the title [CALCITE-5948] Explicit casting should be made if the type of an element in ARRAY/MAP not equals the derived component type [CALCITE-5948] Use explicit casting if element type in ARRAY/MAP does not equal derived component type Sep 22, 2023
@taoran92
taoran92 force-pushed the CALCITE-5948 branch 2 times, most recently from a9befa2 to 90de969 Compare September 22, 2023 09:58
@taoran92
taoran92 requested a review from tanclary September 25, 2023 13:09
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 15 Code Smells

100.0% 100.0% Coverage
5.2% 5.2% Duplication

@taoran92

taoran92 commented Sep 27, 2023 •

Copy link
Copy Markdown
Member Author

hi Tanner @tanclary , if you are free, could you help to take a look again? Sorry to bother you.

@taoran92

taoran92 commented Oct 6, 2023 •

Copy link
Copy Markdown
Member Author

@tanclary Hi, Tanner, I have updated the commit & jira name and added javadocs. If you have time, could you help to review it again? thank you.

@tanclary tanclary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chucheng92 sorry for the delay, I was out unexpectedly for a couple of weeks. Thank you for resolving all of the comments, I think this looks great.

@tanclary
tanclary merged commit 342cf60 into apache:main Oct 9, 2023
// Result should be "EXPR$0=[1, 1.1]\n"; [CALCITE-4850] logged.
CalciteAssert.that()
.query("select array[1, 1.1]")
.returns("EXPR$0=[0E+1, 1.1]\n");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tanclary thanks for patient reviewing! btw, I think we can safely close CALCITE-4850. The result has been corrected.

@FrankChen021

Copy link
Copy Markdown
Member

This change appears to introduce a severe performance regression when validating very large array constructors containing mixed-width string literals.

We reproduced it using Apache Druid’s InPlanningBenchmark.queryStringFunctionInSql:

EXPLAIN PLAN FOR
SELECT COUNT(*)
FROM foo
WHERE long1 = 8
   OR LOWER(string1) IN ('1', '2', ..., '1000000')

Benchmark parameters:

  • inClauseLiteralsCount=1000000
  • inSubQueryThreshold=2147483647
  • rowsPerSegment=500000

Results:

Calcite version Result
1.35.0 10.39 s/op, 14.94 GB allocated/op
1.37.0 No completed operation after 20 minutes
1.41.0 No completed operation after 20 minutes
1.42.0 No completed operation after 20 minutes

The regression first appears after upgrading from Calcite 1.35 to 1.37; this PR was included in Calcite 1.36.

A JFR profile of the 100,000-string case attributed most allocation pressure to:

  • ImmutableList.copyOf: 50.29%
  • Platform.copy: 41.28%

The dominant stack was:

SqlArrayValueConstructor.inferReturnType
  -> SqlValidatorUtil.adjustTypeForArrayConstructor
  -> SqlValidatorUtil.adjustTypeForMultisetConstructor
  -> SqlBasicCall.setOperand
  -> ImmutableNullableList.copyOf

adjustTypeForMultisetConstructor invokes setOperand separately for every element requiring a cast. SqlBasicCall.setOperand copies the complete immutable operand list each time. For large mixed-width string arrays, this produces approximately quadratic copying and extreme allocation pressure.

Numeric arrays generally do not reproduce the same degradation because their elements commonly already have the derived component type.

Related Druid investigation: apache/druid#20326 (comment)

@mihaibudiu

Copy link
Copy Markdown
Contributor

Commenting on a closed PR is not useful, you should open a new issue in JIRA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants