Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

camsaul · 2024-04-26T00:49:22Z

Fixes #8804
Fixes #11643
Fixes #19910
Fixes #39769
Fixes #41863
Fixes #41866
Fixes #42073

The Snowflake JDBC driver still doesn't support Java 8 java.time classes, which means we were setting things like OffsetDateTime as a java.sql.Timestamp... even worse, it ignores attached java.util.Calendars (java.sql.Timestamp + java.util.Calendar is basically the pre-Java-8 way of setting a timestamp with time zone value in JDBC)... we actually had code to do the right thing for Snowflake from #11718 but it only worked for ZonedDateTime:

https://github.com/metabase/metabase/blob/b93e99d/modules/drivers/snowflake/src/metabase/driver/snowflake.clj?rgh-link-date=2024-04-25T21%3A33%3A43Z#L618-L632

In #40530 I fixed related issues for date = date filtering but there were still issues with timestamp_tz = date filtering.

I ended up changing the Snowflake QP code to just output timestamp literals directly, e.g.

'2024-04-25 17:58:00 -07:00'::timestamp_tz

instead of

? -- a java.sql.Timestamp

so we can be sure it's doing the right thing and so we're no longer at the mercy of a poorly-implemented JDBC driver.

I had to fix some stuff with native query parameter compilation, for whatever reason it wasn't calling ->honeysql for java.time classes and just assuming everyone wanted ?; now it's working correctly.

camsaul · 2024-04-26T00:52:26Z

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

-(defmethod unprepare/unprepare-value [:snowflake OffsetDateTime]
-  [_ t]
-  (format "'%s %s %s'::timestamp_tz" (t/local-date t) (t/local-time t) (t/zone-offset t)))
-
-(defmethod unprepare/unprepare-value [:snowflake ZonedDateTime]
-  [driver t]
-  (unprepare/unprepare-value driver (t/offset-date-time t)))
-
-(defmethod unprepare/unprepare-value [:snowflake OffsetTime]
-  [driver t]
-  (unprepare/unprepare-value driver (t/local-time (t/with-offset-same-instant t (t/zone-offset 0)))))


No longer needed since we don't parameterize these anymore.

camsaul · 2024-04-26T00:56:16Z

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

-;; Like Vertica, Snowflake doesn't seem to be able to return a LocalTime/OffsetTime like everyone else, but it can
-;; return a String that we can parse
-
-(defmethod sql-jdbc.execute/read-column-thunk [:snowflake Types/TIMESTAMP_WITH_TIMEZONE]
-  [_ ^ResultSet rs _ ^Integer i]
-  (fn []
-    (when-let [s (.getString rs i)]
-      (let [t (u.date/parse s)]
-        (log/tracef "(.getString rs %d) [TIMESTAMP_WITH_TIMEZONE] -> %s -> %s" i (pr-str s) (pr-str t))
-        t))))
-
-(defmethod sql-jdbc.execute/read-column-thunk [:snowflake Types/TIME]
-  [_ ^ResultSet rs _ ^Integer i]
-  (fn []
-    (when-let [s (.getString rs i)]
-      (let [t (u.date/parse s)]
-        (log/tracef "(.getString rs %d) [TIME] -> %s -> %s" i (pr-str s) (pr-str t))
-        t))))
-
-(defmethod sql-jdbc.execute/read-column-thunk [:snowflake Types/TIME_WITH_TIMEZONE]
-  [_ ^ResultSet rs _ ^Integer i]
-  (fn []
-    (when-let [s (.getString rs i)]
-      (let [t (u.date/parse s)]
-        (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i (pr-str s) (pr-str t))
-        t))))


The TIME impl is exactly the same as the version in metabase.driver.sql-jdbc.execute.legacy-impl, which Snowflake would have used anyway, so I removed it. Snowflake doesn't have TIME_WITH_TIMEZONE, so this was dead code. But metabase.driver.sql-jdbc.execute.legacy-impl was missing TIME_WITH_TIMEZONE and TIMESTAMP_WITH_TIMEZONE impls so I moved them there since they look almost exactly like the other impls.

camsaul · 2024-04-26T00:56:32Z

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

-;; TODO  would it make more sense to use functions like `timestamp_tz_from_parts` directly instead of JDBC parameters?
-
-;; Snowflake seems to ignore the calendar parameter of `.setTime` and `.setTimestamp` and instead uses the session
-;; timezone; normalize temporal values to UTC so we end up with the right values
-(defmethod sql-jdbc.execute/set-parameter [::use-legacy-classes-for-read-and-set java.time.OffsetTime]
-  [driver ps i t]
-  (sql-jdbc.execute/set-parameter driver ps i (t/sql-time (t/with-offset-same-instant t (t/zone-offset 0)))))
-
-(defmethod sql-jdbc.execute/set-parameter [::use-legacy-classes-for-read-and-set java.time.OffsetDateTime]
-  [driver ps i t]
-  (sql-jdbc.execute/set-parameter driver ps i (t/sql-timestamp (t/with-offset-same-instant t (t/zone-offset 0)))))
-
-(defmethod sql-jdbc.execute/set-parameter [:snowflake java.time.ZonedDateTime]
-  [driver ps i t]
-  (sql-jdbc.execute/set-parameter driver ps i (t/sql-timestamp (t/with-zone-same-instant t (t/zone-id "UTC")))))


No longer needed since we're generating SQL literals instead

camsaul · 2024-04-26T00:57:10Z

src/metabase/driver/sql/parameters/substitution.clj

-  (let [[snippet & args] (sql.qp/format-honeysql driver x)]
+  (let [honeysql         (sql.qp/->honeysql driver x)
+        [snippet & args] (sql.qp/format-honeysql driver honeysql)]


Parameter compilation code should call ->honeysql on java.time arguments

camsaul · 2024-04-26T00:57:22Z

src/metabase/driver/sql_jdbc/execute/legacy_impl.clj

  (fn []
    (when-let [s (.getString rs i)]
      (let [t (u.date/parse s)]
        (log/tracef "(.getString rs i) [TIME] -> %s -> %s" (pr-str s) (pr-str t))
        t))))

+(defmethod sql-jdbc.execute/read-column-thunk [::use-legacy-classes-for-read-and-set Types/TIME_WITH_TIMEZONE]


Moved from Snowflake

replay-io · 2024-04-26T01:01:13Z

Status	Complete ↗︎
Commit	`3e0903e`
Results	⚠️ 6 Flaky ✅ 2431 Passed

…ed tests

metamben · 2024-04-26T20:17:54Z

test/metabase/query_processor/test_util.clj

@@ -383,7 +383,7 @@
  also have this issue."
  [driver]
  ;; TIMEZONE FIXME — remove this and fix the drivers
-  (contains? #{:snowflake :oracle :redshift} driver))
+  (contains? #{:oracle :redshift} driver))


The docstring is out of date now.

…olumns

github-actions · 2024-05-01T03:24:29Z

@camsaul Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

… columns (#41864) * Fix Snowflake filtering on date for `timestamp with time zone` columns * Snowflake should just generate raw SQL for literals * Go ahead and fix other Snowflake stuff and reenable a bunch of disabled tests * Snowflake test fixes 🔧 * Minor test improvements * Fix Redshift and Vertica as well. * Address PR feedback

…with time zone columns" (#42088) * Fix Snowflake/Redshift filtering on date for timestamp with time zone columns (#41864) * Fix Snowflake filtering on date for `timestamp with time zone` columns * Snowflake should just generate raw SQL for literals * Go ahead and fix other Snowflake stuff and reenable a bunch of disabled tests * Snowflake test fixes 🔧 * Minor test improvements * Fix Redshift and Vertica as well. * Address PR feedback * Remove test for fix that isn't part of this PR * Remove stuff only in >= 50 * Backport change to skip feature validity check for namespaced driver features --------- Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> Co-authored-by: Cam Saul <github@camsaul.com>

camsaul added 2 commits April 25, 2024 22:39

Fix Snowflake filtering on date for timestamp with time zone columns

f24d35e

Snowflake should just generate raw SQL for literals

994c6e2

metabase-bot bot assigned camsaul Apr 26, 2024

metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label Apr 26, 2024

camsaul added the backport Automatically create PR on current release branch on merge label Apr 26, 2024

camsaul commented Apr 26, 2024

View reviewed changes

Go ahead and fix other Snowflake stuff and reenable a bunch of disabl…

b7e3d1c

…ed tests

metamben approved these changes Apr 26, 2024

View reviewed changes

camsaul added 4 commits April 29, 2024 17:01

Merge branch 'master' into fix-snowflake-filtering-for-timestamp-tz-c…

e97ab7d

…olumns

Merge branch 'master' into fix-snowflake-filtering-for-timestamp-tz-c…

589d38a

…olumns

Snowflake test fixes 🔧

a659256

Minor test improvements

2b10084

camsaul changed the title ~~Fix Snowflake filtering on date for timestamp with time zone columns~~ Fix Snowflake/Redshift filtering on date for timestamp with time zone columns Apr 30, 2024

camsaul added 2 commits April 30, 2024 23:58

Fix Redshift and Vertica as well.

d8652ba

Address PR feedback

5aca3c3

camsaul enabled auto-merge (squash) May 1, 2024 00:21

camsaul added 3 commits May 1, 2024 01:36

Test fixes 🔧

3759a5a

Better Snowflake fix 🔧

11bfe63

More test fixes 🔧

3e0903e

camsaul merged commit 856b449 into master May 1, 2024
108 checks passed

camsaul deleted the fix-snowflake-filtering-for-timestamp-tz-columns branch May 1, 2024 03:24

metabase-bot bot mentioned this pull request May 1, 2024

🤖 backported "Fix Snowflake/Redshift filtering on date for timestamp with time zone columns" #42088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

camsaul commented Apr 26, 2024 •

edited

camsaul Apr 26, 2024

camsaul Apr 26, 2024

camsaul Apr 26, 2024

camsaul Apr 26, 2024

camsaul Apr 26, 2024

replay-io bot commented Apr 26, 2024 •

edited

metamben Apr 26, 2024

github-actions bot commented May 1, 2024

Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

Conversation

camsaul commented Apr 26, 2024 • edited

camsaul Apr 26, 2024

Choose a reason for hiding this comment

camsaul Apr 26, 2024

Choose a reason for hiding this comment

camsaul Apr 26, 2024

Choose a reason for hiding this comment

camsaul Apr 26, 2024

Choose a reason for hiding this comment

camsaul Apr 26, 2024

Choose a reason for hiding this comment

replay-io bot commented Apr 26, 2024 • edited

metamben Apr 26, 2024

Choose a reason for hiding this comment

github-actions bot commented May 1, 2024

camsaul commented Apr 26, 2024 •

edited

replay-io bot commented Apr 26, 2024 •

edited