Skip to content

fix(unparser): Fix BigQuery timestamp literal format in SQL unparsing#21103

Open
sgrebnov wants to merge 3 commits intoapache:mainfrom
spiceai:sgrebnov/1022-bigquery-upstream
Open

fix(unparser): Fix BigQuery timestamp literal format in SQL unparsing#21103
sgrebnov wants to merge 3 commits intoapache:mainfrom
spiceai:sgrebnov/1022-bigquery-upstream

Conversation

@sgrebnov
Copy link
Member

@sgrebnov sgrebnov commented Mar 22, 2026

Which issue does this PR close?

The default Dialect::timestamp_with_tz_to_string uses dt.to_string() which produces timestamps with a space before the TimeZone offset. This causes filter pushdown to fail when unparsing timestamp predicates for BigQuery.

2016-08-06 20:05:00 +00:00 <- invalid for BigQuery:
invalid timestamp: '2016-08-06 20:05:00 +00:00'; while executing the filter on column 'startTime' (query) (sqlstate: [0, 0, 0, 0, 0], vendor_code: -2147483648)

BigQuery rejects this format. Per the BigQuery timestamp docs, the offset must be attached directly to the time:

2016-08-06 20:05:00+00:00 <- valid

What changes are included in this PR?

Following similar to DuckDB pattern/fix override timestamp_with_tz_to_string for BigQueryDialect to produce valid timestamp format

Before (default dt.to_string()):

CAST('2016-08-06 20:05:00 +00:00' AS TIMESTAMP)  -- BigQuery error

After (%:z format):

CAST('2016-08-06 20:05:00+00:00' AS TIMESTAMP)  -- valid BigQuery timestamp

Are these changes tested?

Added unit test and manual e2e test with Google BigQuery instance.

Are there any user-facing changes?

No

@github-actions github-actions bot added the sql SQL Planner label Mar 22, 2026
@sgrebnov sgrebnov marked this pull request as ready for review March 22, 2026 19:13
Copy link
Contributor

@nuno-faria nuno-faria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @sgrebnov.

I wonder if timestamp_with_tz_to_string should use the version without the space as the default, since I think according to ISO 8601 it shouldn't be there.

I tested with PostgreSQL/SQLite/MySQL and they all support the version without the space as well.

@sgrebnov
Copy link
Member Author

sgrebnov commented Mar 26, 2026

dt.to_string()

@nuno-faria - that is a great point, I think we should be using dt.to_rfc3339() by default, which should resolve DuckDB and BigQuery issues as well so we even don't need custom overrides for them. WDYT?

/// Returns an RFC 3339 and ISO 8601 date and time string such as `1996-12-19T16:39:57-08:00`.
    #[cfg(feature = "alloc")]
    #[must_use]
    pub fn to_rfc3339(&self) -> String {
        // For some reason a string with a capacity less than 32 is ca 20% slower when benchmarking.
        let mut result = String::with_capacity(32);
        let naive = self.overflowing_naive_local();
        let offset = self.offset.fix();
        write_rfc3339(&mut result, naive, offset, SecondsFormat::AutoSi, false)
            .expect("writing rfc3339 datetime to string should never fail");
        result
    }

@nuno-faria
Copy link
Contributor

dt.to_string()

@nuno-faria - that is a great point, I think we should be using dt.to_rfc3339() by default, which should resolve DuckDB and BigQuery issues as well so we even don't need custom overrides for them. WDYT?

/// Returns an RFC 3339 and ISO 8601 date and time string such as `1996-12-19T16:39:57-08:00`.
    #[cfg(feature = "alloc")]
    #[must_use]
    pub fn to_rfc3339(&self) -> String {
        // For some reason a string with a capacity less than 32 is ca 20% slower when benchmarking.
        let mut result = String::with_capacity(32);
        let naive = self.overflowing_naive_local();
        let offset = self.offset.fix();
        write_rfc3339(&mut result, naive, offset, SecondsFormat::AutoSi, false)
            .expect("writing rfc3339 datetime to string should never fail");
        result
    }

Sounds good to me. Should we do on this PR or leave as a follow up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants