Skip to content

Add pushdown support for uuidv7 and uuid_extract functions - #449

Open
darshil929 wants to merge 1 commit into
Snowflake-Labs:mainfrom
darshil929:fix/149-uuid-pushdown
Open

Add pushdown support for uuidv7 and uuid_extract functions#449
darshil929 wants to merge 1 commit into
Snowflake-Labs:mainfrom
darshil929:fix/149-uuid-pushdown

Conversation

@darshil929

Copy link
Copy Markdown
Contributor

Description

Adds pushdown support for the PostgreSQL 18 UUID functions uuidv7(), uuid_extract_timestamp() and uuid_extract_version().

The zero-arg uuidv7() is shipped directly since DuckDB's implementation matches PostgreSQL. The two extract functions cannot be shipped as-is because DuckDB's semantics diverge:

  • uuid_extract_timestamp errors in DuckDB for anything that is not v7, while PostgreSQL returns the embedded timestamp for v1 UUIDs and NULL for other versions.
  • uuid_extract_version in DuckDB returns the raw version nibble without the RFC 9562 variant check, so for example a nil UUID yields 0 where PostgreSQL returns NULL.

Following the existing acosh_pg / initcap_pg pattern, queries are rewritten to uuid_extract_timestamp_pg / uuid_extract_version_pg wrappers implemented in duckdb_pglake with the PostgreSQL semantics ported from the PG 18 sources, including the v1 Gregorian timestamp arithmetic and the variant check.

uuidv7(interval) has no DuckDB equivalent and stays unshipped, evaluated by PostgreSQL as before. The shippable entries are gated to PG 18+ so the eager catalog lookup keeps working on PG 16/17.

Tests cover the rewrite in the remote query, result parity between an iceberg table and an identical heap table for v1/v4/v7/nil/max UUIDs, v7 generation via pushed-down uuidv7(), and the interval variant staying local. They skip on PostgreSQL < 18.

closes: #149

Signed-off-by: darshil929 <darshilt.work29@gmail.com>
@sfc-gh-mslot

Copy link
Copy Markdown
Collaborator

Review — and comparison with #173

Thanks for this! I ran the full diff and verified the behaviour on a workspace. This PR (its rebased CI copy is #456) and #173 implement the same feature independently. Overall this is the more correct of the two implementations; my notes and one suggested change below.

What's right here

  • uuid_extract_version is handled correctly. Rewriting to uuid_extract_version_pg with the RFC 9562 variant check is the right call. DuckDB's built-in uuid_extract_version skips the variant check, so it returns 0 for the nil UUID and 54 for the max UUID where Postgres returns NULL. I confirmed this against DuckDB directly. Pushdown uuid functions #173 ships the raw built-in for this function and so returns those wrong values — and its tests don't cover nil/max, so it wouldn't catch it. Your test explicitly covering nil and max is exactly what's needed.
  • The v1 Gregorian-epoch and v7 ms→µs arithmetic is correct, and the UnaryExecutor::ExecuteWithNulls implementation is clean.
  • Test coverage (variant matrix + heap-vs-iceberg parity + uuidv7(interval) staying local) is good.

One correctness fix to make: PG 17

The gating and the comments assume these functions are "Postgres 18+ only", but that's not accurate:

  • uuid_extract_timestamp (v1 only) and uuid_extract_version exist in PG 17.
  • uuidv7, and v7 support in uuid_extract_timestamp, arrive in PG 18.

So gating everything at PG_VERSION_NUM >= 180000 isn't wrong in the sense of producing bad results — on PG 17 the calls just fall back to local Postgres execution — but it silently forgoes pushdown on PG 17. Please:

  1. Lower the shippable + rewrite gate for the two uuid_extract_* functions to PG 17+, keeping uuidv7 at 18+, and fix the "only exist in Postgres 18+" comments.
  2. Because on PG 17 a v7 UUID must return NULL from uuid_extract_timestamp (v7 timestamps aren't extracted until 18), uuid_extract_timestamp_pg needs to be version-aware for this to be safe. Pushdown uuid functions #173 does this by taking PG_VERSION_NUM as a second argument and returning NULL for v7 when pg_version < 180000. Adopting that shape here is the cleanest way to extend to PG 17 without regressing.

Net: this PR's variant-checked version wrapper + #173's version-parameterised timestamp wrapper + PG 17 coverage is the ideal combined result.

Minor

  • Version-file collision to be aware of: this PR uses pg_lake_engine--3.3--3.4.sql; Pushdown uuid functions #173 uses --3.4--3.5.sql. Whichever merges second will need a rebase onto the new version.

RETURNS smallint
LANGUAGE C
IMMUTABLE PARALLEL SAFE STRICT
AS 'MODULE_PATHNAME', $function$pg_lake_internal_dummy_function$function$;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should move into 3.4--3.5.sql now, 3.4 has been released

@darshil929

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review and the comparison, really appreciate the time spent on it. Good catch on the PG 17 gating, I had wrongly assumed the extract functions were new in 18 along with uuidv7. The version argument approach looks like the right way to handle it. Let me know if anything is needed from my side.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pushdown uuidv7 functions

2 participants