Skip to content

[CALCITE-6471] Improve performance of SqlToRelConverter by preventing unconditional conversion of sqlNodes to string for null-check messages #3884

Merged
asolimando merged 1 commit intoapache:mainfrom
korlov42:calcite-6471
Aug 8, 2024

Conversation

@korlov42
Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/CALCITE-6471

This patch replaces all unconditional conversions of SqlNode to string in SqlToRelConverter with on-demand message suppliers for null checks (requireNonNull).

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 25, 2024
Copy link
Copy Markdown
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

LGTM

@mihaibudiu
Copy link
Copy Markdown
Contributor

Can you please update the commit message to match the JIRA subject?

@mihaibudiu mihaibudiu changed the title [CALCITE-6471] On-demand null-check message creation in SqlToRelConverter [CALCITE-6471] Improve performance of SqlToRelConverter by preventing unconditional conversion of sqlNodes to string for null-check messages Aug 1, 2024
@korlov42
Copy link
Copy Markdown
Contributor Author

korlov42 commented Aug 8, 2024

Folks, thank you all for review! I've got 3 approvals from reviewers as well as one from sonarcloud. Also it has label LGTM-will-merge-soon. But the PR is stuck in this state for a few weeks, so... What should I do next in order to drive this forward?

@asolimando
Copy link
Copy Markdown
Member

@korlov42, can you update the (single) commit message to match the updated title of the Jira ticket? After that I think we can merge the PR.

… unconditional conversion of sqlNodes to string for null-check messages
@korlov42
Copy link
Copy Markdown
Contributor Author

korlov42 commented Aug 8, 2024

@asolimando , done. I've also rebased my branch on latest main.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Aug 8, 2024

@asolimando asolimando merged commit 2d6e9a7 into apache:main Aug 8, 2024
@korlov42 korlov42 deleted the calcite-6471 branch August 8, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants