Fix query failure of predicate pushdown to BigQuery DATETIME type #9005

ebyhr · 2021-08-27T08:40:28Z

The query having filter on BigQuery DATETIME type caused below error before this change:

trino> select * from bigquery.test.test_datetime where col = cast('0001-01-01 00:00:00.999' as timestamp(3));
Query 20210827_070648_00014_u3ikv failed: 
io.grpc.StatusRuntimeException: INVALID_ARGUMENT: request failed: 
Row filter for xxx.test.test_datetime is invalid. Filter is '(`col` = '+1967030-04-28 09:35:38.000')'
...

Run all BigQuery tests locally (passed)

hashhar · 2021-08-27T11:54:06Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryType.java

@@ -147,7 +147,7 @@ static String dateToStringConverter(Object value)

    static String datetimeToStringConverter(Object value)
    {
-        return formatTimestamp(((Long) value).longValue(), systemDefault());
+        return formatTimestamp(((Long) value).longValue() / NANOSECONDS_PER_MICROSECOND, UTC);


Does BigQuery connector only read TIMESTAMP(3)? Otherwise the Object can be a long-timestamp type which is not backed by a long.

If I understand correctly, this method is called only when building filter and not used for reading BigQuery data. If we specify long-timestamp in condition, this method won't be called because the precision is different.

Thanks, indeed since we only have type-mapping for timestamp/tz millis we'll always get a long here.

Can you add a checkState/verify here + a test that tries to use TIMESTAMP(6) so that once we support higher precisions the test will fail and we'll be reminded to correct this code?

Otherwise we may forget about this.

(If you are already working on higher precision timestamp support then feel free to ignore since there's a lower chance of forgetting this).

Alternative, you can com.google.common.math.LongMath#divide(value, 1000, RoundingMode.UNNECESSARY) so that the code doesn't do silent data conversion for precision >3.

NANOSECONDS_PER_MICROSECOND -> MICROSECONDS_PER_MILLI, i guess

@hashhar Let me skip applying comments about timestamp(6) negative tests since I will fix soon.

@findepi Thanks, fixed.

findepi · 2021-08-27T14:39:43Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryType.java

@@ -147,7 +147,7 @@ static String dateToStringConverter(Object value)

    static String datetimeToStringConverter(Object value)
    {
-        return formatTimestamp(((Long) value).longValue(), systemDefault());
+        return formatTimestamp(((Long) value).longValue() / NANOSECONDS_PER_MICROSECOND, UTC);


Alternative, you can com.google.common.math.LongMath#divide(value, 1000, RoundingMode.UNNECESSARY) so that the code doesn't do silent data conversion for precision >3.

findepi · 2021-08-27T14:40:00Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryType.java

@@ -147,7 +147,7 @@ static String dateToStringConverter(Object value)

    static String datetimeToStringConverter(Object value)
    {
-        return formatTimestamp(((Long) value).longValue(), systemDefault());
+        return formatTimestamp(((Long) value).longValue() / NANOSECONDS_PER_MICROSECOND, UTC);


NANOSECONDS_PER_MICROSECOND -> MICROSECONDS_PER_MILLI, i guess

findepi · 2021-08-27T14:40:27Z

.../trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java

+    {
+        String tableName = "test.test_datetime_type";
+
+        onBigQuery("DROP TABLE IF EXISTS " + tableName);


this can run on a shared resources, so table name should be randomized

I remember there was opposite comment (should be fixed) by other people in the past. While I can understand both opinion, BigQuery has expiration_timestamp table option. I think we can use this option to cleanup automatically. Of course we can improve by try-with-resources, but it's not perfect solution.
I will simply change to randomized names anyway in this PR.

findepi · 2021-08-27T14:41:12Z

.../trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java

+
+        assertQuery(
+                "SELECT a FROM " + tableName,
+                "VALUES (CAST('2021-08-27 12:34:56.123' AS TIMESTAMP(3))), (CAST('2022-09-28 01:02:03.987' AS TIMESTAMP(3)))");


CAST('2022-09-28 01:02:03.987' AS TIMESTAMP(3)) is very verbose way of writing TIMESTAMP '2022-09-28 01:02:03.987'

for this, instead of h2 , use

assertThat(query( trino query using big query ) .matches ( trino query using values )

especially when working with date/time things.

findepi · 2021-08-27T14:41:30Z

.../trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java

+                "SELECT a FROM " + tableName + " WHERE a = CAST('2021-08-27 12:34:56.123' AS TIMESTAMP(3))",
+                "VALUES (CAST('2021-08-27 12:34:56.123' AS TIMESTAMP(3)))");
+        assertQuery(
+                "SELECT a FROM " + tableName + " WHERE rand() = 42 OR a = CAST('2021-08-27 12:34:56.123' AS TIMESTAMP(3))",


the case with rand() tests the engine, not the connector. i feel like you can remove it.

ebyhr · 2021-08-30T01:08:49Z

@findepi @hashhar Applied comments and confirmed TestBigQueryIntegrationSmokeTest passes locally.

findepi · 2021-08-30T08:53:03Z

.../trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java

+                ImmutableList.of("'2021-08-27 12:34:56.123'", "'2022-09-28 01:02:03.987'"))) {
+            assertThat(query("SELECT a FROM " + table.getName()))
+                    .matches("VALUES TIMESTAMP '2021-08-27 12:34:56.123', TIMESTAMP '2022-09-28 01:02:03.987'");
+            assertThat(query("SELECT a FROM " + table.getName() + " WHERE a = TIMESTAMP '2021-08-27 12:34:56.123'"))


any chance to have io.trino.testing.AbstractTestDistributedQueries#testDataMappingSmokeTest run for BQ?
it would probably cover this

BQ connector doesn't support writes

Could it support writes for test purposes?

Storage write API is in the beta preview right now

ebyhr · 2021-08-30T11:15:49Z

Let me merge PR though of course I agree adding type mapping tests eventually.

cla-bot bot added the cla-signed label Aug 27, 2021

ebyhr added the bug Something isn't working label Aug 27, 2021

ebyhr requested review from martint, wendigo and hashhar August 27, 2021 08:41

wendigo approved these changes Aug 27, 2021

View reviewed changes

hashhar reviewed Aug 27, 2021

View reviewed changes

hashhar approved these changes Aug 27, 2021

View reviewed changes

findepi reviewed Aug 27, 2021

View reviewed changes

hashhar self-requested a review August 27, 2021 16:05

Fix query failure of predicate pushdown to BigQuery DATETIME type

69a683c

ebyhr force-pushed the ebi/bigquery-datetime-pushdown branch from bffbc1a to 69a683c Compare August 30, 2021 00:52

findepi approved these changes Aug 30, 2021

View reviewed changes

ebyhr merged commit 9024438 into trinodb:master Aug 30, 2021

ebyhr deleted the ebi/bigquery-datetime-pushdown branch August 30, 2021 11:16

ebyhr added this to the 362 milestone Aug 30, 2021

ebyhr mentioned this pull request Aug 30, 2021

Release notes for 362 #9015

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix query failure of predicate pushdown to BigQuery DATETIME type #9005

Fix query failure of predicate pushdown to BigQuery DATETIME type #9005

ebyhr commented Aug 27, 2021 •

edited

hashhar Aug 27, 2021

ebyhr Aug 27, 2021

hashhar Aug 27, 2021

findepi Aug 27, 2021

findepi Aug 27, 2021

ebyhr Aug 30, 2021

findepi Aug 27, 2021

findepi Aug 27, 2021

findepi Aug 27, 2021

ebyhr Aug 27, 2021 •

edited

findepi Aug 27, 2021

findepi Aug 27, 2021

ebyhr commented Aug 30, 2021

findepi Aug 30, 2021

wendigo Aug 30, 2021

findepi Aug 30, 2021

wendigo Aug 30, 2021

ebyhr commented Aug 30, 2021

Fix query failure of predicate pushdown to BigQuery DATETIME type #9005

Fix query failure of predicate pushdown to BigQuery DATETIME type #9005

Conversation

ebyhr commented Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Aug 30, 2021

ebyhr commented Aug 27, 2021 •

edited

ebyhr Aug 27, 2021 •

edited