[CALCITE-5944] Add metadata for Sample #3382

JiajunBernoulli · 2023-08-20T09:37:00Z

No description provided.

LakeShen · 2023-08-20T23:20:56Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdMaxRowCount.java

@@ -104,6 +105,10 @@ public class RelMdMaxRowCount
    return mq.getMaxRowCount(rel.getInput());
  }

+  public @Nullable Double getMaxRowCount(Sample rel, RelMetadataQuery mq) {
+    return mq.getMaxRowCount(rel.getInput());


Hi,I have a question is that why getMaxRowCount method not consider the sample rate?

Bernoulli sampling can, in the worst case, return all rows, or no rows.

Not sure about system sampling.

LakeShen · 2023-08-20T23:22:15Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdMinRowCount.java

@@ -103,6 +104,10 @@ public Double getMinRowCount(Filter rel, RelMetadataQuery mq) {
    return mq.getMinRowCount(rel.getInput());
  }

+  public @Nullable Double getMinRowCount(Sample rel, RelMetadataQuery mq) {
+    return mq.getMinRowCount(rel.getInput());


Same as above.

LakeShen · 2023-08-20T23:22:39Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdRowCount.java

@@ -169,6 +170,12 @@ public Double getRowCount(Calc rel, RelMetadataQuery mq) {
    return limit < rowCount ? limit : rowCount;
  }

+  public @Nullable Double getRowCount(Sample rel, RelMetadataQuery mq) {


This is make sense for me.

Yes. Perhaps worth justifying with a comment (because nothing is obvious in statistics). The expected value of a binomial distribution with probability p over n trials is n * p.

LakeShen · 2023-08-20T23:24:30Z

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java

+   * <a href="https://issues.apache.org/jira/browse/CALCITE-5944">[CALCITE-5944]
+   * RelMdRowCount can return more accurate rowCount. </a>. */
+  @Test void testRowCountSample() {
+    sql("select * from emp tablesample bernoulli(50) repeatable(1)")


Could you add the test for getMaxRowCount(Sample rel, RelMetadataQuery mq) and getMinRowCount(Sample rel, RelMetadataQuery mq)?

Too many '.'s.

Also, the jira summary is not very descriptive. This is about adding support for Sample, right?

Sorry, I missing Sample.

JiajunBernoulli · 2023-08-21T06:01:29Z

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java

+   * RelMdRowCount can return more accurate rowCount. </a>. */
+  @Test void testRowCountSample() {
+    sql("select * from emp tablesample bernoulli(50) repeatable(1)")
+        .assertThatRowCount(is(EMP_SIZE * 0.5), is(0D), is(Double.POSITIVE_INFINITY));


@LakeShen
The second parameter 0D is MinRowCount.
The third parameter Double.POSITIVE_INFINITY is MaxRowCount.

So we have test for getMaxRowCount(Sample rel, RelMetadataQuery mq) and getMinRowCount(Sample rel, RelMetadataQuery mq).

@LakeShen The second parameter 0D is MinRowCount. The third parameter Double.POSITIVE_INFINITY is MaxRowCount.

So we have test for getMaxRowCount(Sample rel, RelMetadataQuery mq) and getMinRowCount(Sample rel, RelMetadataQuery mq).

Hi @JiajunBernoulli ,thank you for reminding me.

JiajunBernoulli · 2023-08-26T09:42:42Z

The unit test case is unstable.


FAILURE 311.7sec, org.apache.calcite.adapter.tpch.TpchTest > testQuery07()

    java.util.concurrent.TimeoutException: testQuery07() timed out after 5 minutes

        at org.junit.jupiter.engine.extension.TimeoutExceptionFactory.create(TimeoutExceptionFactory.java:29)

        at org.junit.jupiter.engine.extension.SameThreadTimeoutInvocation.proceed(SameThreadTimeoutInvocation.java:58)

        at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)

        at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)

        at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)

        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)

        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)

        at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)

        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)

        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)

        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)

        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)

        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)

        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:217)

        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)

        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:213)

        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:138)

        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:68)

        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)

        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)

        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)

        at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)

        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)

        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)

        at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)

JiajunBernoulli · 2023-09-01T02:20:40Z

@rubenada , would you please review it?

rubenada · 2023-09-04T08:21:21Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdRowCount.java

@@ -169,6 +170,12 @@ public Double getRowCount(Calc rel, RelMetadataQuery mq) {
    return limit < rowCount ? limit : rowCount;
  }

+  public @Nullable Double getRowCount(Sample rel, RelMetadataQuery mq) {
+    final Double rowCount = mq.getRowCount(rel.getInput());


nitpick: I'd name this variable inputRowCount or something like that.
Also, as Julian mentions, perhaps a comment would be helpful.

rubenada · 2023-09-04T08:23:12Z

@JiajunBernoulli overall lgtm, I left a minor comment.

- RelMdAllPredicates - RelMdColumnOrigins - RelMdExpressionLineage - RelMdMaxRowCount - RelMdMinRowCount - RelMdPredicates - RelMdRowCount

JiajunBernoulli · 2023-09-07T02:05:43Z

Rename inputRowCount and rebase main.

@rubenada Thank you~

sonarqubecloud · 2023-09-07T02:06:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

LakeShen reviewed Aug 20, 2023

View reviewed changes

JiajunBernoulli changed the title ~~[CALCITE-5944] RelMdRowCount can return more accurate rowCount~~ [CALCITE-5944] RelMdRowCount can return more accurate rowCount for Sample Aug 21, 2023

JiajunBernoulli commented Aug 21, 2023

View reviewed changes

JiajunBernoulli added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 2, 2023

rubenada changed the title ~~[CALCITE-5944] RelMdRowCount can return more accurate rowCount for Sample~~ [CALCITE-5944] Add metadata for Sample Sep 4, 2023

rubenada reviewed Sep 4, 2023

View reviewed changes

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

[CALCITE-5944] Add metadata for Sample

92757f3

- RelMdAllPredicates - RelMdColumnOrigins - RelMdExpressionLineage - RelMdMaxRowCount - RelMdMinRowCount - RelMdPredicates - RelMdRowCount

JiajunBernoulli force-pushed the CALCITE-5944 branch from 08089ab to 92757f3 Compare September 7, 2023 01:46

JiajunBernoulli merged commit 64268b9 into apache:main Sep 8, 2023

[CALCITE-5944] Add metadata for Sample #3382

[CALCITE-5944] Add metadata for Sample #3382

Uh oh!

Conversation

JiajunBernoulli commented Aug 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiajunBernoulli commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiajunBernoulli commented Sep 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenada commented Sep 4, 2023

Uh oh!

JiajunBernoulli commented Sep 7, 2023

Uh oh!

sonarqubecloud bot commented Sep 7, 2023

Uh oh!

Uh oh!

JiajunBernoulli commented Aug 26, 2023 •

edited

Loading