You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "jduo (via GitHub)" <gi...@apache.org> on 2024/01/17 00:26:20 UTC
[PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
jduo opened a new pull request, #3632:
URL: https://github.com/apache/calcite/pull/3632
(From: #3151)
This PR corrects a row count call to use the RelMetadataQuery instead of going directly to rel.estimateRowCount.
Using rel.estimateRowCount directly will behave incorrectly when using a cardinality estimator that is implemented entirely in a MetadataHandler class, as it will evade the metadata handler's implementation of getRowCount and fall through to the underlying default method for Values.
This corrects the problem by using mq.getRowCount instead of rel.estimateRowCount. In the default case this results in the same value but in the custom cardinality model case the expected handler is called.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1468033950
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
+ // Arrange
+ final String sql = "values(1,2,3)";
+ final RelNode rel = sql(sql).toRel();
+ assertThat(rel, instanceOf(Values.class));
+
+ RelMetadataProvider provider = RelMdPopulationSize.SOURCE;
+
+ List<MetadataHandler<?>> handlers =
+ provider.handlers(BuiltInMetadata.PopulationSize.Handler.class);
+
+ // Act
Review Comment:
Comments should provide information that is not clear from the code. "Assert" for example does not. And "act" could mean many thing. I think it would be better without them.
But here the test is not showing that you fixed a bug, it is showing just that a specific method is called. The bug should manifest itself in producing an incorrect or suboptimal result for some code transformation. That would be a convincing test.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894779707
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1460518848
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
+ // Arrange
+ final String sql = "values(1,2,3)";
+ final RelNode rel = sql(sql).toRel();
+ assertThat(rel, instanceOf(Values.class));
+
+ RelMetadataProvider provider = RelMdPopulationSize.SOURCE;
+
+ List<MetadataHandler<?>> handlers =
+ provider.handlers(BuiltInMetadata.PopulationSize.Handler.class);
+
+ // Act
Review Comment:
This is a pretty common way of documenting tests:
https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/
- Arrange would be setting up the test (that isn't global setup like the @Setup function)
- Act would be executing the test behavior
- Assert would be validating the result.
I can remove these comments but don't really see the harm in them.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1922050930
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915767458
You are describing how something is implemented. A bug description should be something like "Query X produces result Y instead of the expected Z", or "Optimizer produces the plan X instead of the expected Y because it uses the wrong statistics".
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada merged PR #3632:
URL: https://github.com/apache/calcite/pull/3632
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915785837
> You are describing how something is implemented. A bug description should be something like "Query X produces result Y instead of the expected Z", or "Optimizer produces the plan X instead of the expected Y because it uses the wrong statistics".
I guess in this case, the original implementation happens to return the correct value, but is not behaving as a valid extension point. So I'm not sure what would be a good concrete test for this type of problem.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1926433238
Thanks @jduo , I have updated the Jira title myself.
@mihaibudiu , @JiajunBernoulli I think the PR is in a good shape, do you have further remarks? Otherwise I'll merge it in the next 24-48h
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1912690078
@mihaibudiu I've changed the test. It's still a bit artificial because it makes a RelMetadataQuery which intentionally reports an incorrect statistic but hopefully demonstrates how the bug is fixed. Hope the comments are clear enough.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1912533333
Hi @mihaibudiu and @JiajunBernoulli , just wondering what the next steps are with this PR.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1454319255
##########
core/src/test/java/org/apache/calcite/rel/metadata/RelMdPopulationSizeTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rel.metadata;
+
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.hep.HepPlanner;
+import org.apache.calcite.plan.hep.HepProgram;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for {@link RelMdPopulationSize}.
+ */
+public class RelMdPopulationSizeTest {
+
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
Can we add it in `RelMetadataTest`?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1456054855
##########
core/src/test/java/org/apache/calcite/rel/metadata/RelMdPopulationSizeTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rel.metadata;
+
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.hep.HepPlanner;
+import org.apache.calcite.plan.hep.HepProgram;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for {@link RelMdPopulationSize}.
+ */
+public class RelMdPopulationSizeTest {
+
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
Done.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1460133533
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
This isn't really displaying a bug that is being fixed.
From the JIRA comments I understand that it's difficult to create a small reproduction.
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
+ // Arrange
+ final String sql = "values(1,2,3)";
+ final RelNode rel = sql(sql).toRel();
+ assertThat(rel, instanceOf(Values.class));
+
+ RelMetadataProvider provider = RelMdPopulationSize.SOURCE;
+
+ List<MetadataHandler<?>> handlers =
+ provider.handlers(BuiltInMetadata.PopulationSize.Handler.class);
+
+ // Act
Review Comment:
I am not sure that these 3 comments are useful.
In fact, this one confused me.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894809290
@jduo Yes, you can `squash` after any committer approve.
But now we maybe need some new commits to make PR better.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1923356817
@jduo, usually the commit message is the same as Jira subject (more info here: https://calcite.apache.org/develop/#contributing).
What if we use (for both Jira and commit) something that describes in-between the problem and the change, i.e. `[CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq)` ?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1912755097
Forgot to add @adamkennedy to this PR.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1470092970
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
Yes, the test is still artificial.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894734702
@libenchao @chunweilei
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1927540121
I don't have any comments.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1899425810
@JiajunBernoulli @mihaibudiu , were there any other concerns to address in this PR?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1912654400
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1921499432
LGTM.
@jduo , Just to avoid confusion, would it be possible to align commit message with PR title and Jira title?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894756811
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1460520468
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
The bug is that the population size was being calculated without utilizing RelMetadataQuery#getRowCount(). The idea was that this test demonstrates the getRowCount() call is being used.
Does that clarify the bug and purpose of the test? This sort of test you'd probably want to spy() instead but I didn't want to introduce Mockito into the core sub-module just for this.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1460520468
##########
core/src/test/java/org/apache/calcite/test/RelMetadataTest.java:
##########
@@ -3639,6 +3642,37 @@ public void checkAllPredicatesAndTableSetOp(String sql) {
is(mq.getPopulationSize(rel, bitSetOf(0))));
}
+ /**
+ * Test that RelMdPopulationSize is calculated based on the RelMetadataQuery#getRowCount().
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-5647">[CALCITE-5647]</a>
+ */
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
@mihaibudiu The bug is that the population size was being calculated without utilizing RelMetadataQuery#getRowCount(). The idea was that this test demonstrates the getRowCount() call is being used.
Does that clarify the bug and purpose of the test? This sort of test you'd probably want to spy() instead but I didn't want to introduce Mockito into the core sub-module just for this.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894742398
> There are some style violations
Thanks. Should be correct now.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894738317
There are some style violations
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894795481
Thanks @JiajunBernoulli.
Do you mean I should squash my commits and give it the [CALCITE-5647] prefix?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915761224
> I am approving, but I am still unsure about exactly what this improvement fixes.
The intent with RelMetadataQuery is that the implementation can provide custom behavior for each of the statistics you can requet with it.
For the case of the population size of Values clauses, it should be based on the row count / 2. However RelMetadataQuery was not using invoking its (potentially custom) method to get the row count, it was just inlining the row count estimate from the node.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1915788785
Supposedly you have changed this behavior because it causes some problems in some application someone cares about. The ideal test case would be a small reproduction of the bug which caused this quest in the first place. The JIRA cases suggests that a small reproduction is difficult.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1921996618
@rubenada , I've made the commit and PR have the same title. Should the JIRA have the same title now? It states the problem whereas the commit/PR describe what changed.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1924809312
> @jduo, usually the commit message is the same as Jira subject (more info here: https://calcite.apache.org/develop/#contributing).
>
> What if we use (for both Jira and commit) something that describes in-between the problem and the change, i.e. `[CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq)` ?
Sounds good. I've updated the name of the PR and the commit message. However I cannot seem to edit the JIRA title (perhaps because I'm not the original reporter of the issue).
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5647] RelMdPopulationSize should use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1924835521
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3632&resolved=false&inNewCodePeriod=true)
[100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3632&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3632)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1894792002
@jduo Thanks for your PR.
Just minor suggestion:
1. Commit message should start with `[CALCITE-5647]`.
2. We like adding Java Document and JIRA link before unit test.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on PR #3632:
URL: https://github.com/apache/calcite/pull/3632#issuecomment-1896208862
> @jduo Thanks for your PR.
>
> Just minor suggestion:
>
> 1. Commit message should start with `[CALCITE-5647]`.
> 2. We like adding Java Document and JIRA link before unit test.
Done.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org