fix bug join query with require time condition by EungsopYoo · Pull Request #14890 · apache/druid

EungsopYoo · 2023-08-22T02:32:04Z

Description

Fixed bug join query with require time condition

If druid.sql.planner.requireTimeCondition is set true, the query fails but every subqueries has the filter on __time column.

SELECT a.__time, a.cnt, b.cnt
FROM (
  SELECT __time, SUM(cnt) cnt
  FROM druid.foo
  WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
  GROUP BY __time
) a
JOIN (
  SELECT __time, SUM(cnt) cnt
  FROM druid.foo
  WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
  GROUP BY __time
) b on (a.__time = b.__time)

Error: Unknown exception

requireTimeCondition is enabled, all queries must include a filter condition on the __time column

org.apache.druid.sql.calcite.rel.CannotBuildQueryException

This fails too.

SELECT a.__time, a.cnt, b.cnt
FROM (
  SELECT __time, SUM(cnt) cnt
  FROM druid.foo
  WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
  GROUP BY __time
) a
JOIN (
  SELECT __time, SUM(cnt) cnt
  FROM druid.foo
  WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
  GROUP BY __time
) b on (a.__time = b.__time)
WHERE a.__time >= TIMESTAMP '2000-01-01 00:00:00' AND a.__time < TIMESTAMP '2000-01-02 00:00:00'

So I fixed that find the base intervals of left and right data sources, instead of outer query, when the dataSource is a instance of JoinDataSource.

Choice of algorithms
- If dataSource is a instance of JoinDataSource, find the base intervals of left and right data sources.

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

abhishekagarwal87 · 2023-08-22T08:10:03Z

can you explain what the bug is?

somu-imply · 2023-08-22T16:14:22Z

Can you please add an example as in what was not working and what is the bug

EungsopYoo · 2023-08-23T00:37:59Z

Description updated

adarshsanjeev · 2023-09-28T16:31:07Z

Hey @EungsopYoo! Thanks for the PR, I'm going through it currently. Could you please resolve the conflict with master?

LakshSingla

Thanks for the PR. I have added a review regarding the testing methodology and code practices we follow.
Though I haven't looked at the changes, why do you think JOIN queries should be exempted from a __time filter if the subqueries have the filter?
If that's the case, then say something like UNION should also be exempt from the restriction.

LakshSingla · 2023-10-10T05:22:37Z

...ore/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java

+
+  @Ignore
+  @Override
+  public void testEquiJoin2()
+  {
+
+  }
+
+  @Ignore
+  @Override
+  public void testEquiJoin3()
+  {
+
+  }
+
+  @Ignore
+  @Override
+  public void testEquiJoin4()
+  {
+
+  }


Can you add a comment on why these joins are not working with MSQ? Per our knowledge, MSQ supports joins and hence the inability to work with one is concerning and should be documented.

There's a msqIncompatible flag that you can use instead, which is for this purpose itself unless this error happens in the planning phase.

LakshSingla · 2023-10-10T05:23:24Z

sql/src/test/java/org/apache/druid/sql/calcite/DecoupledPlanningCalciteQueryTest.java

+  @Override
+  @Ignore
+  public void testEquiJoin2()
+  {
+
+  }


Try using @DecoupledIgnore annotation on the original test method.

LakshSingla · 2023-10-10T05:24:03Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

The tests should be moved to CalciteJoinQueryTest

LakshSingla · 2023-10-10T05:25:01Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

  }
+
+  @Test
+  public void testEquiJoin()


Also, can you please use descriptive names for the test, where they represent what the test is primarily supposed to be verifying, or documents the regression why the test was added.

For example: testEquiJoinWhereLeftHandIsConstant

EungsopYoo · 2023-10-18T00:59:56Z

Thanks for the PR. I have added a review regarding the testing methodology and code practices we follow. Though I haven't looked at the changes, why do you think JOIN queries should be exempted from a __time filter if the subqueries have the filter?

Actually join queries inherit __time filters from their sub queries, not exempt the __time filtering.

If that's the case, then say something like UNION should also be exempt from the restriction.

I think UNION ALL works basically well when requireTimeCondition=true.

SELECT __time, SUM(cnt) cnt
FROM druid.foo
WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
GROUP BY __time
UNION ALL
SELECT __time, SUM(cnt) cnt
FROM druid.foo
WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
GROUP BY __time

// OK

SELECT __time, SUM(cnt) cnt
FROM druid.foo
GROUP BY __time
UNION ALL
SELECT __time, SUM(cnt) cnt
FROM druid.foo
WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
GROUP BY __time

// Error: Unknown exception
// requireTimeCondition is enabled, all queries must include a filter condition on the __time column
// org.apache.druid.sql.calcite.rel.CannotBuildQueryException

But, its error message is a little weird in this condition.

SELECT __time, SUM(cnt) cnt
FROM druid.foo
WHERE __time >= TIMESTAMP '2000-01-01 00:00:00' AND __time < TIMESTAMP '2000-01-02 00:00:00'
GROUP BY __time
UNION ALL
SELECT __time, SUM(cnt) cnt
FROM druid.foo
GROUP BY __time

// Query results were truncated midstream! This may indicate a server-side error or a client-side issue. Try re-running your query, or using a lower limit or a longer timeout.

LakshSingla · 2023-11-21T09:21:32Z

I revisited the PR, and it seems that you have added a condition on the Join clause on the top datasource, however the join datasource can be nested however deep, and we'd need to take that into account while performing the check. Something like the following structure


     Q
      |
     Q
     ...
     J
   /.    \
  Q.    .....
  |
  Q
 /. \
Da Db

abhishekagarwal87 · 2023-11-21T10:18:39Z

sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java

+        DataSource rightInner = ((QueryDataSource) right).getQuery().getDataSource();
+        DataSourceAnalysis rightAnlaysisInner = rightInner.getAnalysis();
+        if (rightInner instanceof JoinDataSource) {
+          return findMaxBaseDataSourceIntervals(rightInner, rightAnlaysisInner, defaults);


The right thing is to ensure that both left and right have time filters. Since this method is used only to check the eternity interval, it could be refactored to checkThatDataSourceHaveFiniteInterval and then the method throws an exception wherever there is an eternity interval list. and that check should be performed on children.
what do you think?

There is also a DruidQuery#getFiltration method that checks and creates filtration on a data source. We can refactor the whole thing and move the check to there instead as well, however, that would need further testing because we don't want the method's semantics to change. Also, it would only be applicable to SQL queries.

Also, what's the purpose of the requireTimeCondition? If it is to prevent the query from hitting all of the segments on the historicals, then it probably doesn't do a very good job of it, and perhaps can must be changed. The problems I see with it:

Doesn't take into account the data source type. IMO should only be affecting the table data source since other data sources don't involve segments.

Even if the SQL query has a time filtration, it isn't guaranteed that the data source has the time filtration as well. Ideally, it should, but there was a bug in UNNEST (earlier) that didn't push down the filtration to the data source, and the query would end up reading the whole data source. Perhaps other nuances in translation might also cause the underlying data source to not have a time filtration.

github-actions · 2024-02-28T00:15:45Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2024-03-28T00:16:09Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

kfaraz added Bug Area - Querying labels Aug 22, 2023

EungsopYoo force-pushed the fix-join-with-requireTimeCondition branch 2 times, most recently from 9777412 to 32c440d Compare August 24, 2023 06:29

fix bug join query with require time condition

35eca2b

EungsopYoo force-pushed the fix-join-with-requireTimeCondition branch from 32c440d to 35eca2b Compare August 24, 2023 06:30

Merge branch 'master' into fix-join-with-requireTimeCondition

d83d0c6

github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Oct 4, 2023

fixup

828b5c9

LakshSingla reviewed Oct 10, 2023

View reviewed changes

abhishekagarwal87 reviewed Nov 21, 2023

View reviewed changes

github-actions bot added the stale label Feb 28, 2024

github-actions bot closed this Mar 28, 2024

Comments

Conversation

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

Uh oh!

Description

Fixed bug join query with require time condition

Uh oh!

abhishekagarwal87 commented Aug 22, 2023

Uh oh!

somu-imply commented Aug 22, 2023

Uh oh!

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

Uh oh!

Uh oh!

adarshsanjeev commented Sep 28, 2023

Uh oh!

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

LakshSingla Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

LakshSingla Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

LakshSingla Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

LakshSingla Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

EungsopYoo commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LakshSingla commented Nov 21, 2023

Uh oh!

abhishekagarwal87 Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

LakshSingla Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

LakshSingla Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 28, 2024

Uh oh!

github-actions bot commented Mar 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

EungsopYoo commented Aug 22, 2023 •

edited

Loading

EungsopYoo commented Aug 23, 2023 •

edited

Loading

EungsopYoo commented Oct 18, 2023 •

edited

Loading