Skip to content

[CALCITE-6650] Optimize the IN sub-query and SOME sub-query by Metada…#4022

Merged
NobiGo merged 1 commit into
apache:mainfrom
NobiGo:CALCITE-6650
Nov 11, 2024
Merged

[CALCITE-6650] Optimize the IN sub-query and SOME sub-query by Metada…#4022
NobiGo merged 1 commit into
apache:mainfrom
NobiGo:CALCITE-6650

Conversation

@NobiGo

@NobiGo NobiGo commented Oct 29, 2024

Copy link
Copy Markdown
Contributor

…ta RowCount

// FALSE.
final RelMetadataQuery mq = e.rel.getCluster().getMetadataQuery();
final Double maxRowCount = mq.getMaxRowCount(e.rel);
if (maxRowCount != null && maxRowCount < 1D) {

@dssysolyatin dssysolyatin Oct 29, 2024

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.

Are you sure that maxRowCount < 1D guarantees that the subquery will return zero records?

Since maxRowCount is a Double, I think if maxRowCount = 0.7 for example, there is a possibility that it could return more than 0 records.

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.

I have the same concern as @dssysolyatin. Could we instead check whether it is an instance of Values and then call Values.isEmpty? (Which is how PruneEmptyRules work.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have rule in PruneEmptyRules that does:

          return maxRowCount != null && maxRowCount == 0.0;

Given that comparisons with doubles are inherently imprecise we may need to revisit that rule as well. Should we employ a threshold comparison?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In addition, it seems that we employ the maxRowCount < 1D check in other places as well so whatever we decide we may need to propagate the changes everywhere.

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.

The semantics of this value should be documented. Is it always guaranteed to be less than the actual row count of the table?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zabetak Perhaps we should clarify more clearly that when 'maxRowCount==0D' means no data is returned?

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.

Sure. If you can find a better place to put the logic at

final Double minRowCount = mq.getMinRowCount(e.rel);
then do it. Maybe a method @Nullable Boolean RelMetadataQuery.isEmpty(RelNode) (where true means always empty, false means always non-empty, null for everything else).

I cannot imagine a case where a provider would return a maxRowCount value less than one where the relation was not guaranteed empty, so I wouldn't worry too much whether the condition is < 1D or == 0D.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think add @Nullable Boolean RelMetadataQuery.isEmpty(RelNode) is a good way for users to customize their implementation. The Default behavior will use maxRowCount == 0.0.

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.

Note that we don't allow people to override methods in RelMetadataQuery. If we want people to be able to customize we should create a new kind of metadata.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have created an issue CALCITE-6665 to log it.

@NobiGo NobiGo added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Nov 7, 2024

@mihaibudiu mihaibudiu 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.

This looks fine to me. Unless there are objections in the next few days I plan to merge this.

@mihaibudiu

Copy link
Copy Markdown
Contributor

Feel free to squash the commits and merge if there are no other suggestions.

@sonarqubecloud

Copy link
Copy Markdown

@NobiGo
NobiGo merged commit f347fba into apache:main Nov 11, 2024
@NobiGo
NobiGo deleted the CALCITE-6650 branch November 15, 2024 11:12
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.

6 participants