Implement timestamp predicate pushdown in Druid connector by jerryleooo · Pull Request #8474 · trinodb/trino

jerryleooo · 2021-07-06T09:07:58Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/pom.xml

hashhar

Please add tests. It's difficult to prevent regressions or verify if it's working as expected without it.

Take a look at TestPostgreSqlTypeMapping.
You'll need to modify the tests to perform insertions using the JDBC driver instead of using Trino (since Druid connector doesn't support writes yet).

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

sumannewton · 2021-07-07T07:38:12Z

@jerryleooo Edit the PR to Fix #8474: Pushdown timestamp filters in druid connector

jerryleooo · 2021-07-07T07:52:07Z

Fix #8474: Pushdown timestamp filters in druid connector

OK sure but I think it should be Fix #8404: Pushdown timestamp filters in druid connector since 8474 is this PR self

jerryleooo · 2021-07-07T08:41:28Z

You'll need to modify the tests to perform insertions using the JDBC driver instead of using Trino (since Druid connector doesn't support writes yet).

JDBC driver may also not work -- in druid they ingest data with:

trino/plugin/trino-druid/src/test/java/io/trino/plugin/druid/TestingDruidServer.java

Line 235 in 13c3a72

void ingestData(String datasource, String indexTaskFile, String dataFilePath)

jerryleooo · 2021-07-07T15:57:40Z

Hi @hashhar some latest progress:

It seems Druid is OK with the withRounding version, and for sure I need to add tests
Then I found it is difficult to add tests like other JDBC connectors, since Druid doesn't support creating tables and insert data on the fly (which is needed in TestDruidTypeMapping), we can only "ingest data", I am trying to see if there is an easier way. Your advice is most welcome.
Thanks

hashhar · 2021-07-08T05:02:07Z

Seems like we cannot "write" data at all to Druid. Creating ingest jobs for each possible test table doesn't scale too.

@wendigo @Parth-Brahmbhatt @dheerajkulakarni Any ideas how we can work around the Druid limitations to ingest some test data without having to create TSV files and batch ingestion jobs from those?

dheerajkulakarni · 2021-07-10T08:09:34Z

Hey @hashhar @jerryleooo , I think I have done something similar in this pull request . I created a method in which I was reading from a datasource and pushing it to other datasource with some other name.
Please check copyAndIngestTpchData ovverrided method in this file in my pull request if it helps.

jerryleooo · 2021-07-11T16:53:59Z

Thank @dheerajkulakarni for the idea, however, I think it has a prerequisite -- the target data source has the same schema as the source data source, so we can just change the ioConfig part and reuse the index task file. I don't think it works for randomly generated data. (correct me if I miss something)

I guess the final solution would be generating index task file and io file, copy the io file into container, and then do the ingestion.

jerryleooo · 2021-07-14T08:27:15Z

Hi @hashhar @findepi @dheerajkulakarni
As I have mentioned, since Druid doesn't support CREATE TABLE and INSERT DATA, it is not very easy to add the type mapping test.

I tried to add a Druid version of CreateAndInsertDataSetup and enhanced the TestTable constructor. Until now I still faced the issue of the index task failure -- in /druid/indexer/v1/task/{taskId}/status API, the runnerStatusCode is always WAITING although the status is SUCCESS. Can share your ideas if you know what's the cause.

UPDATE: temporarily solved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

hashhar

@jerryleooo Can we extract the testing related changes to a separate PR and get that merged first?

Thanks a lot for coming up with the approach of generating both data and the indexing tasks dynamically. Will make it possible to test and hence implement more features in Druid connector.

jerryleooo · 2021-07-15T15:16:51Z

Thanks @hashhar for the review.
I am not sure if I should have another PR, since the type mapping test seems also to help prove my changes are correct.
And for sure if that makes things easier I would like to do that.
For now, I think all the functions and basic tests are done.

hashhar · 2021-07-15T15:32:50Z

@jerryleooo That's a fair point. I agree now. But once we think everything looks good we should at-least move that into a separate commit.

I'll review this in a while.

plugin/trino-druid/src/test/java/io/trino/plugin/druid/DruidSqlDataTypeTest.java

hashhar

Some initial comments.

Let's break into commits as:

The public to private change of DRUID_SCHEMA.
Introduction of the Druid test setup (TestTable, CreateAndInsertDataSetup etc.)
Adding explicit timestamp mapping and tests for timestamp predicate pushdown and type mapping tests for timestamp. (Let's handle other types as follow-ups to keep scope limited and easier to reason about and review).

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

hashhar · 2021-07-16T06:04:12Z

plugin/trino-druid/src/test/java/io/trino/plugin/druid/BaseDruidConnectorTest.java

+    @Test
+    public void testPredicatePushdown()
+    {
+        assertThat(query("SELECT * FROM orders where __time > DATE'1970-01-01'")).isFullyPushedDown();


Copy the tests from TestPostgreSqlConnectorTest#testPredicatePushdown and remove the ones not relevant for Druid. Keeping tests consistent is useful and allows extracting them to base test classes in the future.

For __time pushdown add a separate test method since that's something specific to Druid.

hashhar · 2021-07-16T06:08:09Z

testing/trino-testing/src/main/java/io/trino/testing/sql/TestTable.java

@@ -35,35 +35,49 @@ public class TestTable



It might make more sense to introduce a new TestTable within the trino-druid module. After all other modules shouldn't be affected by Druid's limitations.

Please extract to separate class.

The consideration was there might be other databases that don't support creating tables and inserting data -- but you are right, this might be a premature abstraction. WIll modify.

plugin/trino-druid/src/test/resources/ingestion-index.tpl

plugin/trino-druid/src/test/java/io/trino/plugin/druid/ingestion/TestIndexTaskBuilder.java

plugin/trino-druid/src/test/java/io/trino/plugin/druid/TestingDruidServer.java

plugin/trino-druid/src/test/java/io/trino/plugin/druid/DruidSqlDataTypeTest.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

findepi · 2021-07-22T08:04:50Z

plugin/trino-druid/src/test/java/io/trino/plugin/druid/DruidSqlDataTypeTest.java

+import static java.util.stream.Collectors.joining;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public final class DruidSqlDataTypeTest


What would it take to avoid copying SqlDataTypeTest?

SqlDataTypeTest relies on TestTable, in whose constructor CREATE TABLE and INSERT DATA will be run -- this is not suitable for Druid.
#8404 (comment)

There are 3 things in the game: SqlDataTypeTest, DataSetup and TestTable

SqlDataTypeTest delegates all the setup to DataSetup, so only this needs to be specific for Druid

TestTable is used in DataSetup interface

SqlDataTypeTest uses TestTable for two things:

knowing table name

removing table with TestTable#close (cleanup)

We should address this by introducing a new interface:

call it TemporaryRelation

with methods getRelationName and close() (extending from AutoCloseable)

make TestTable implement TemporaryRelation

use it in DataSetup interface definition

all current DataSetup implementations will continue to use TestTable

you will write a new implementation for Druid.

Would that work?

cc @hashhar

findepi · 2021-07-22T08:10:16Z

https://github.com/trinodb/trino/pull/8474/files#r673634986 seems to suggest all Druid timestamps have millisecond precision (for storage)
#8474 (comment) seems to suggest that Druid supports nanosecond precision (for queries at least).

If Druid's storage is always millisecond precision, then our life is simpler. We can easily support millisecond precision, without any rounding, with full pushdown.
We just need a column mapping for this that would

map to trino timestamp(3) type
does not round, but ensures the read value fits in millisecond precision (has zero nanoseconds-of-millisecond)
does full pushdown

lkm · 2021-10-21T07:03:27Z

Seems like this has run aground, is there anything I can help with to get it moving again? The current state of the Druid connector makes a quite limited for real-life purposes and this seems like the first thing to tackle even before aggs, which sadly is also stranded.

jerryleooo · 2021-10-22T03:01:27Z

@lkm I will make time to finish this.

Praveen2112 · 2022-07-12T14:25:23Z

@jerryleooo Any updates on this PR ? If you are okay, can I re-work on this PR and fix the issue ?

Praveen2112 · 2022-07-29T06:27:33Z

Thanks @jerryleooo for working on this PR. I have applied your changes as a part of this PR - #13335.

jerryleooo · 2022-07-29T06:39:38Z

@Praveen2112 cool bro and sorry for the late reply

cla-bot bot added the cla-signed label Jul 6, 2021

jerryleooo marked this pull request as draft July 6, 2021 09:08

jerryleooo mentioned this pull request Jul 6, 2021

Druid __time filter is not pushed down #8404

Closed

sumannewton reviewed Jul 6, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

sumannewton reviewed Jul 6, 2021

View reviewed changes

plugin/trino-base-jdbc/pom.xml Outdated Show resolved Hide resolved

jerryleooo force-pushed the fix_8404 branch from 27da7b6 to 5442131 Compare July 6, 2021 12:16

jerryleooo marked this pull request as ready for review July 7, 2021 01:21

jerryleooo requested review from hashhar and sumannewton July 7, 2021 01:21

hashhar reviewed Jul 7, 2021

View reviewed changes

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java Outdated Show resolved Hide resolved

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java Outdated Show resolved Hide resolved

sumannewton reviewed Jul 7, 2021

View reviewed changes

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java Outdated Show resolved Hide resolved

sumannewton reviewed Jul 7, 2021

View reviewed changes

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java Outdated Show resolved Hide resolved

jerryleooo changed the title ~~try fixing #8404~~ Fix #8474: Pushdown timestamp filters in druid connector Jul 7, 2021

jerryleooo changed the title ~~Fix #8474: Pushdown timestamp filters in druid connector~~ Fix #8404: Pushdown timestamp filters in druid connector Jul 7, 2021

hashhar changed the title ~~Fix #8404: Pushdown timestamp filters in druid connector~~ Implement timestamp predicate pushdown in Druid connector Jul 7, 2021

jerryleooo marked this pull request as draft July 14, 2021 09:34

jibinpt reviewed Jul 14, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java Show resolved Hide resolved

hashhar reviewed Jul 15, 2021

View reviewed changes

jerryleooo force-pushed the fix_8404 branch 2 times, most recently from ac96a1c to 83d7bed Compare July 15, 2021 15:11

jerryleooo marked this pull request as ready for review July 15, 2021 15:12

hashhar self-requested a review July 15, 2021 15:32

jerryleooo commented Jul 16, 2021

View reviewed changes

plugin/trino-druid/src/test/java/io/trino/plugin/druid/DruidSqlDataTypeTest.java Show resolved Hide resolved

hashhar reviewed Jul 16, 2021

View reviewed changes

findepi reviewed Jul 22, 2021

View reviewed changes

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

jerryleooo mentioned this pull request Nov 29, 2021

Resolve code style errors during compile. jerryleooo/trino#2

Closed

jerryleooo force-pushed the fix_8404 branch from 83d7bed to 9fe354d Compare December 1, 2021 04:27

jerryleooo marked this pull request as draft December 2, 2021 04:03

jerryleooo added 2 commits January 5, 2022 15:43

fix 8404

f99c75c

resolve conflicts

3a5096a

jerryleooo force-pushed the fix_8404 branch from b582cae to 3a5096a Compare January 5, 2022 07:48

jerryleooo added 4 commits January 5, 2022 15:52

use nanos

307d5ad

fix compile error

cecb031

fix checkArgument

3993c2e

keep it mills for now

a6eb0b6

Praveen2112 closed this Jul 29, 2022

Conversation

jerryleooo commented Jul 6, 2021 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sumannewton commented Jul 7, 2021

Uh oh!

jerryleooo commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerryleooo commented Jul 7, 2021

Uh oh!

jerryleooo commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashhar commented Jul 8, 2021

Uh oh!

dheerajkulakarni commented Jul 10, 2021

Uh oh!

jerryleooo commented Jul 11, 2021

Uh oh!

jerryleooo commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

jerryleooo commented Jul 15, 2021

Uh oh!

hashhar commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

hashhar Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

jerryleooo Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

jerryleooo Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

findepi Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

findepi commented Jul 22, 2021

Uh oh!

lkm commented Oct 21, 2021

Uh oh!

jerryleooo commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerryleooo commented Jul 6, 2021 •

edited by findepi

Loading

jerryleooo commented Jul 7, 2021 •

edited

Loading

jerryleooo commented Jul 7, 2021 •

edited

Loading

jerryleooo commented Jul 14, 2021 •

edited

Loading

hashhar commented Jul 15, 2021 •

edited

Loading

jerryleooo commented Oct 22, 2021 •

edited

Loading