rubenada commented on PR #3674:
URL: https://github.com/apache/calcite/pull/3674#issuecomment-1933577073
@mihaibudiu this change is subtle, but it has an impact (although we do not
see it in Calcite tests).
By default, this change will have zero effect, because Calcite's default
implementation of `mq.getRowCount(rel)` simply calls
`rel.estimateRowCount(mq)`; so it seems we are getting to the same place with
one extra step; and indeed we are (that's the reason why we do not see any
impact on our tests).
However, the fact of using `mq.getRowCount(rel)` allows downstream projects
to plug their own metadata rowCount and "override" the default rowCount
computation proposed by Calcite. If we call directly
`rel.estimateRowCount(mq)`, this flexibility is lost. In fact, this PR is just
applying our own Calcite rules, see `RelNode#estimateRowCount` javadoc:
```
Don't call this method directly. Instead, use
{@link RelMetadataQuery#getRowCount}, which gives plugins a chance to
override the rel's default ideas about row count.
```
So all these calls to `rel.estimateRowCount(mq)` should have been
`mq.getRowCount(rel)` from the beginning (and we are likely dealing with a
copy-paste of sub-optimal code propagated). As I said before, at that time this
might have been difficult to catch, because by default the effect is the same.
@tanclary @JiajunBernoulli Writing tests for these changes is possible, but
cumbersome. See e.g. the recent commit
https://github.com/apache/calcite/commit/f7069cc5245c22f816c565669f52b4f30b046f4d
which performed a similar change in one location, and how "artificial" it was
to show the impact on a unit test.
Personally, I understand the logic behind this change, and bearing in mind
that is not straightforward to provide tests for this, I'd consider
exceptionally merging the PR without tests. But if others consider that this is
not acceptable, I'll accept that decision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
