more TIME_NS support (v1.4) by jraymakers · Pull Request #19613 · duckdb/duckdb

jraymakers · 2025-11-02T19:05:21Z

#19606 rebased to v1.4-andium.

Differences:

No changes to Shell required (relevant code is only in main).
Some tests that only exist in main (parquet/variant) didn't need changes in this PR.
Some (straightforward) fixes were needed to variant conversion in this branch.

pdet

Hi @jraymakers thanks for the PR, is looking good!

Just had a couple minor comments.

src/function/table/arrow_conversion.cpp

pdet · 2025-11-05T09:59:23Z

test/api/capi/test_capi.cpp

+	// time_ns columns
+	REQUIRE_NO_FAIL(tester.Query("CREATE TABLE time_ns_values(d TIME_NS)"));
+	REQUIRE_NO_FAIL(tester.Query("INSERT INTO time_ns_values VALUES ('12:34:56.789123456'), (NULL), ('12:34:56')"));
+
+	result = tester.Query("SELECT * FROM time_ns_values ORDER BY d");
+	REQUIRE_NO_FAIL(*result);
+	REQUIRE(result->IsNull(0, 0));
+	duckdb_time_ns time_ns_val = result->Fetch<duckdb_time_ns>(0, 1);
+	REQUIRE(time_ns_val.nanos == 45296000000000);
+	REQUIRE(result->Fetch<string>(0, 1) == Value::TIME_NS(dtime_ns_t(45296000000000)).ToString());
+	time_ns_val = result->Fetch<duckdb_time_ns>(0, 2);
+	REQUIRE(time_ns_val.nanos == 45296789123456);
+	REQUIRE(result->Fetch<string>(0, 2) == Value::TIME_NS(dtime_ns_t(45296789123456)).ToString());
+


Can we add tests for the other conversion values as well?

Which other conversion values are you referring to? I tried to cover all the same cases as the similar code above, testing TIME.

This PR implements the conversion for many different precisions, no?

Ah, I assume you mean the new case for TIME_NS I added to ColumnArrowToDuckDB.

It's true this case, like the one for TIME it is patterned after, implements conversions from all four precision levels defined by Arrow (second, millisecond, microsecond, and nanosecond). But, since DuckDB only writes microseconds for TIME, and (now) nanoseconds for TIME_NS, it's not clear to me how to exercise the other code paths. The existing "Arrow roundtrip" test infrastructure can only converts from Arrow to DuckDB what is converted from DuckDB to Arrow, and I'm not aware of any way to generate the second or millisecond precision levels through this path. There don't appear to be any existing tests for these paths for TIME.

With some effort, it might be possible to write additional test infrastructure that could test non-roundtrip Arrow conversions, such as writing microseconds (using TIME) and then reading nanoseconds (using TIME_NS), or vice versa. But I don't feel confident in my ability to implement that infrastructure; in this PR, I'm just trying to plug some gaps in TIME_NS support by closely following existing patterns for TIME.

jraymakers · 2025-11-07T20:10:44Z

@pdet Thanks for the review. I'd love to get this in for 1.4.2 (scheduled for next week), to unblock TIME_NS support in Node Neo (and other users of the C API). Let me know if you think the remaining issue about the test coverage of Arrow conversions for other precision levels is a blocker, or whether it can wait for later.

Tishj · 2025-11-10T09:47:19Z

src/function/cast/variant/to_json.cpp

+	case VariantLogicalType::TIME_NANOS: {
+		auto val = Value::TIME_NS(Load<dtime_ns_t>(ptr));
+		auto val_str = val.ToString();
+		return yyjson_mut_strncpy(doc, val_str.c_str(), val_str.size());


This is untested, no?

This is covered by test/sql/types/variant/test_all_types.test. Without this addition, that test (now) fails with INTERNAL Error: VariantLogicalType(21) not handled, because of the addition of time_ns to test_all_types (in this PR).

Tishj · 2025-11-10T09:50:34Z

test/sql/function/list/list_intersect.test


 statement ok
-CREATE TABLE all_types AS SELECT * FROM test_all_types();
+CREATE TABLE all_types AS SELECT * EXCLUDE(time_ns) FROM test_all_types();


I don't think just excluding it is the right move

pdet · 2025-11-10T10:00:28Z

Hi @jraymakers, after chatting with @Tishj, the basic conclusion is that we need to remove all test exclusions of time_ns, and that the remaining missing tests are a blocker, especially for the release of a minor version.

As it stands, there are untested code paths with potential undesirable side effects.

jraymakers · 2025-11-10T15:37:17Z

I hear you that the exclusions are not desirable. They were not my first choice; I added them because I couldn't figure out another path forward.

I want to point out that the exclusions don't reduce test coverage. They are only necessary because of the change in this PR that adds a time_ns column to test_all_types. Today, there is no such column. So the exclusions simply maintain the status quo, for the subset of tests that can't handle the new type, and for which this is not clear (to me) how to fix.

Here are some possible paths forward:

Help me understand how to avoid the test exclusions while maintaining the functionality (i.e. the time_ns column in test_all_types). The challenge (as I understand it) is the relevant tests are BWC tests, and this column doesn't exist in the old versions but does in the new ones.
Merge this PR as-is, with the exclusions that maintain test coverage at (no less than) current levels, and work on improving both TIME_NS support (e.g. in conversions and aggregate functions) and TIME_NS test coverage in future PRs.
Split this PR into parts, so the most critical pieces, which fix the broken TIME_NS type in both the C API and Arrow conversion (and hence Wasm), can be merged for 1.4.2, without the addition of time_ns to test_all_types.

I'm open to any of these paths, but I'm doubtful I'll be unable to accomplish the first on my own.

jraymakers · 2025-11-10T15:48:14Z

Note that my reason for expanding the scope of this PR to include the addition to test_all_types was to increase test coverage, because so many existing tests use test_all_types, including the Arrow roundtrip tests. If I split the PR, it would be hard to achieve the same level of coverage in each part without the addition to test_all_types.

pdet · 2025-11-11T09:27:38Z

Note that my reason for expanding the scope of this PR to include the addition to test_all_types was to increase test coverage, because so many existing tests use test_all_types, including the Arrow roundtrip tests. If I split the PR, it would be hard to achieve the same level of coverage in each part without the addition to test_all_types.

Hi @jraymakers, I think your reasoning makes perfect sense. Either @Tishj or I can pick it up and fix the test for all types.

The last thing is that it’s not possible to get this PR into v1.4.2, considering it’s a feature and we still need to implement the aforementioned missing tests first. I understand it’s necessary for TIME_NS support in Node Neo, but again, this is not a bug, and we should refrain from adding features in minor releases.

I think the way to move forward is to get your latest changes to #19606, close this PR and continue from there.

jraymakers · 2025-11-11T17:03:13Z

I'd argue there is a bug, or several:

Running select '00:00:00'::time_ns::variant; in the CLI results in INTERNAL Error: VariantLogicalType(21) not handled.
Running select '00:00:00'::time_ns; in DuckDB Wasm (e.g. https://shell.duckdb.org/) results in {"exception_type":"Not implemented","exception_message":"Unsupported Arrow type TIME_NS"}.
Any query returning a TIME_NS will run into problems in the C API because it doesn't handle that type. (In Node Neo, this manifests as Error: Unexpected type id: 0.)

But, it's your call, and I can understand how adding a new column to test_all_types in a patch release could be disruptive. I'll port my changes over to the main PR so we can move forward.

jraymakers · 2025-11-13T17:40:25Z

I've updated the main version of this PR: #19606

pdet · 2025-11-14T09:11:23Z

Thanks @jraymakers I'll close this one for now in favor of #19606
I'll have a look on the missing tests next week!

This was referenced Nov 2, 2025

more TIME_NS support #19606

Closed

TIME_NS support duckdb/duckdb-node-neo#329

Merged

Support TIME_NS data type (new in 1.4.0) duckdb/duckdb-node-neo#295

Open

duckdb-draftbot marked this pull request as draft November 4, 2025 07:16

jraymakers marked this pull request as ready for review November 4, 2025 07:17

pdet self-requested a review November 5, 2025 09:41

pdet requested changes Nov 5, 2025

View reviewed changes

pdet added the Changes Requested label Nov 5, 2025

jraymakers added 5 commits November 7, 2025 11:10

more TIME_NS support

f2693ae

fix arrow_roundtrip and slow tests

7e9f11f

fix variant conversion

d6df20f

fix tests by excluding time_ns

ff29fe4

use AllValid in some arrow conversion functions

0bb38f4

jraymakers force-pushed the jray/more-time-ns-support-v1.4 branch from f7be3d5 to 0bb38f4 Compare November 7, 2025 19:59

duckdb-draftbot marked this pull request as draft November 7, 2025 19:59

jraymakers marked this pull request as ready for review November 7, 2025 20:00

formatting

f7a5e84

duckdb-draftbot marked this pull request as draft November 7, 2025 20:05

jraymakers marked this pull request as ready for review November 7, 2025 20:05

jraymakers mentioned this pull request Nov 9, 2025

bump pkgs to 1.4.1-r.5 duckdb/duckdb-node-neo#334

Merged

Tishj reviewed Nov 10, 2025

View reviewed changes

pdet closed this Nov 14, 2025

Conversation

jraymakers commented Nov 2, 2025

Uh oh!

pdet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pdet Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

jraymakers Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

pdet Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

jraymakers Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

jraymakers commented Nov 7, 2025

Uh oh!

Tishj Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

jraymakers Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Tishj Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

pdet commented Nov 10, 2025

Uh oh!

jraymakers commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jraymakers commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pdet commented Nov 11, 2025

Uh oh!

jraymakers commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jraymakers commented Nov 13, 2025

Uh oh!

pdet commented Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jraymakers commented Nov 10, 2025 •

edited

Loading

jraymakers commented Nov 10, 2025 •

edited

Loading

jraymakers commented Nov 11, 2025 •

edited

Loading