Skip to content

feat(data): support writing TIME_NS values - #387

Merged
Giorgi merged 3 commits into
Giorgi:developfrom
Mr444-dev:feat/time-ns-write-support
Nov 5, 2026
Merged

Giorgi merged 3 commits into
Giorgi:developfrom
Mr444-dev:feat/time-ns-write-support

Conversation

@Mr444-dev

@Mr444-dev Mr444-dev commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add TIME_NS write support for the existing Data-layer temporal types: TimeOnly, DateTime, DuckDBTimeOnly, and TimeSpan parameters declared as DbType.Time.
  • Centralize .NET tick conversion in DuckDBTimeNs.FromTicks in Bindings.
  • Keep DuckDBTimeNs as the native TIME_NS layout; it is not exposed as a Data-layer value, parameter, reader result, or function result.
  • Add regression coverage for appender and mapped-appender writes, lists (including nullable and empty lists), parameters, sub-microsecond precision, and 24:00. Expected values are compared using DuckDB SQL values.

Validation

  • Focused TIME_NS write tests: 5 passed, repeated five times.
  • Standard suite excluding VarintTest and connection stress tests per AGENTS.md: 1,691 passed, 1 skipped.
  • Data project Release build: succeeded with 0 warnings and 0 errors.
  • git diff --check passed.

@coveralls

coveralls commented Nov 3, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 42059113838

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.6%) to 93.39%

Details

  • Coverage increased (+0.6%) from the base build.
  • Patch coverage: 13 of 13 lines across 5 files are fully covered (100%).
  • 52 coverage regressions across 14 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

52 previously-covered lines in 14 files lost coverage.

Top 10 Files by Coverage Loss Lines Losing Coverage Coverage
DuckDB.NET.Data/DataChunk/Writer/VectorDataWriterBase.cs 23 82.55%
DuckDB.NET.Data/DataChunk/Reader/DecimalVectorDataReader.cs 5 92.35%
DuckDB.NET.Data/DataChunk/Reader/VectorDataReaderBase.cs 4 92.99%
DuckDB.NET.Bindings/DuckDBHugeInt.cs 3 92.5%
DuckDB.NET.Data/DuckDBMappedAppender.cs 3 84.62%
DuckDB.NET.Data/Mapping/DuckDBAppenderMap.cs 2 98.0%
DuckDB.NET.Data/DuckDBConnection.ScalarFunction.Extensions.cs 2 94.77%
DuckDB.NET.Data/DuckDBAppenderRow.cs 2 96.04%
DuckDB.NET.Bindings/DuckDBWrapperObjects.cs 2 89.77%
DuckDB.NET.Data/DataChunk/Reader/BigIntegerVectorDataReader.cs 2 96.27%

Coverage Stats

Coverage Status
Relevant Lines: 4575
Covered Lines: 4319
Line Coverage: 94.4%
Relevant Branches: 3019
Covered Branches: 2773
Branch Coverage: 91.85%
Branches in Coverage %: Yes
Coverage Strength: 1059905.08 hits per line

💛 - Coveralls

@Giorgi

Giorgi commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Thanks for picking up the write side. Please change these three things:

1. The mapped appender still refuses TIME_NS. GetAcceptedDuckDBTypes in DuckDBMappedAppender.cs maps TimeOnly / DuckDBTimeOnly to Time only, so CreateAppender<T, TMap> throws a type mismatch ("expected DuckDB type: Time but actual column type is TimeNs") for a TimeOnly property mapped to a TIME_NS column, while the plain appender writes the same value. Please accept TimeNs for those types too, the same way BigInteger accepts both HugeInt and UnsignedHugeInt, and add a mapped-appender test.

2. Compare with DuckDB, not with constants worked out in C#. The header of TemporalWriteConversionTests says "Do not replace the comparisons with constants worked out in C#", but the new tests check against strings such as "12:34:56.1234567". If the shared nanosecond constant were wrong in both directions, or DuckDB changed how it stores or prints TIME_NS, those constants could still match. Please have DuckDB work out the expected value from the same time, like the TIME test in this class does across the day. For example, for many tick values, write the value and check in SQL that stored = '<the time as text>'::TIME_NS, so DuckDB parses the expected value itself.

3. TimeSpan with DbType.Time still loses sub-microsecond digits. The new (DuckDBType.TimeNs, ...) cases in ClrToDuckDBConverter cover DateTime, DuckDBTimeOnly and TimeOnly. A TimeSpan parameter with DbType.Time bound to a TIME_NS parameter still goes through ValueCreators[DbType.Time], which sends a microsecond TIME. So TimeSpan.FromTicks(452_961_234_567) arrives as 12:34:56.123456, while the same time as TimeOnly arrives as 12:34:56.1234567. The reader returns TIME_NS as TimeSpan too, so a round trip through TimeSpan loses digits. Please add a (DuckDBType.TimeNs, TimeSpan) case (a time of day, refusing a negative value or one of a day or more as the TIME path does) and a test.

@Giorgi

Giorgi commented Nov 3, 2026

Copy link
Copy Markdown
Owner

One more thing: the write tests cover only part of TimeNsVectorDataWriter. Please add tests for these:

  • List items of every type the writer takes. Only List<TimeOnly> is appended now. Please also append List<DateTime> and List<DuckDBTimeOnly> items, a list with a NULL item (List<TimeOnly?>), and 24:00:00 inside a list (DuckDBTimeOnly(24, 0, 0)).
  • Function results. Scalar and table function output go through the same writer factory. Please add a scalar function and a table function that return TIME_NS (with a sub-microsecond value and 24:00).
  • The empty-list row. times[1] of an empty list is NULL whatever the writer does, because that's DuckDB's list indexing. Please check the list itself instead, for example len(times) = 0.

Also, please put the new tests next to the TIME write tests rather than at the top of the class, and split them into smaller tests (appender, list, parameter, 24:00). In one large method, the first failing assertion hides the rest.

@Giorgi

Giorgi commented Nov 4, 2026

Copy link
Copy Markdown
Owner

One more: please keep the TIME_NS conversion in one place for both directions.

The read side already has it on DuckDBTimeNs in Bindings (Ticks => Nanos / 100). The write side adds three ToDuckDBTimeNs extensions in Data's DateTimeExtensions, each multiplying ticks by NanosecondsPerTick. So the factor is written twice (a literal 100 in Bindings, the constant in Data) and could drift apart. The TimeOnly and DuckDBTimeOnly overloads are also the same expression, value.Ticks * NanosecondsPerTick.

Please add the inverse of Ticks next to it, for example public static DuckDBTimeNs FromTicks(long ticks) => new() { Nanos = ticks * 100 };, and use it in TimeNsVectorDataWriter and ClrToDuckDBConverter. Then the three extensions can go: DuckDBTimeNs.FromTicks(time.Ticks) for TimeOnly and DuckDBTimeOnly, and DuckDBTimeNs.FromTicks(dateTime.Ticks % TimeSpan.TicksPerDay) for DateTime. (Bindings can't see Data's internal constants, and Data references Bindings, so the public struct is the place both sides can use.)

@Giorgi

Giorgi commented Nov 4, 2026

Copy link
Copy Markdown
Owner

@Mr444-dev Can you make these changes? 👆

@Mr444-dev

Copy link
Copy Markdown
Contributor Author

I addressed the requested changes in commit 7b96ab7:

  • TimeOnly and DuckDBTimeOnly mapped appenders now accept TIME_NS columns, with regression coverage.
  • The tests cover every requested list item type, NULL and empty lists, 24:00, and scalar and table function outputs.
  • Expected time values are parsed by DuckDB in SQL comparisons. TimeSpan parameters targeting TIME_NS now preserve ticks and reject values outside a day.
  • Tick-to-nanosecond conversion is centralized in DuckDBTimeNs.FromTicks.

The focused TIME_NS tests passed 12/12 on five consecutive runs. The standard suite passed 1,693 tests with one skipped, excluding VarintTest and connection stress tests per AGENTS.md. The new build workflow is still running; CodeQL has passed. I also updated the type-mapping docs in Giorgi/DuckDB.NET-Docs#9.

@Giorgi

Giorgi commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Thanks, most of the requested changes are in. A few smaller things:

  1. Test placement. The TIME_NS tests are still at the top of TemporalWriteConversionTests. Please move them next to the TIME write tests.
  2. The table function test's check. time_value = CASE WHEN time_value = ... THEN ... ELSE ... END is hard to follow. Please check it the way the scalar function test does: join the rows to a VALUES list of the expected times and compare each row with its expected value.
  3. checked in DuckDBTimeNs.FromTicks. None of the values the writers and the converter pass in can overflow: TimeOnly and DateTime % TicksPerDay stay under a day, and DuckDBTimeOnly reaches 24:00 at most. So the check never fires there, but it runs on every value written. Please drop it, which also matches Ticks.

@Giorgi

Giorgi commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Please remove the DuckDBTimeNs support from Data. It's the raw TIME_NS layout from Bindings, like DuckDBTime for TIME. DuckDBTime isn't accepted anywhere in Data: you can't append it, bind it, return it from a function or read it with GetFieldValue. The types users work with for TIME_NS are TimeOnly, TimeSpan, DateTime and DuckDBTimeOnly, the same as for TIME.

Please take out:

  • { typeof(DuckDBTimeNs), DuckDBType.TimeNs } in TypeExtensions
  • AppendTimeNs and its two dispatch lines in VectorDataWriterBase, and the override in TimeNsVectorDataWriter
  • AppendValue(DuckDBTimeNs?) in DuckDBAppenderRow and IDuckDBAppenderRow (adding a member to the public interface also breaks anyone who implements it)
  • the DuckDBTimeNs? result line in DuckDBConnection.ScalarFunction.Extensions.cs
  • the (DuckDBType.TimeNs, DuckDBTimeNs value) case in ClrToDuckDBConverter
  • the two DuckDBTimeNs branches in DateTimeVectorDataReader
  • the tests that use it: the nanosecondTime rows in the appender and parameter tests, and the scalar and table function tests

As it stands, the support is also incomplete:

  • a mapped appender with a DuckDBTimeNs property is accepted when it's created, then AppendRecords throws NotSupportedException ("Type DuckDBTimeNs is not supported for appending")
  • a bare SELECT ? parameter with a DuckDBTimeNs value arrives as VARCHAR
  • in a table function, parameters[0].GetValue<DuckDBTimeNs>() on TIME_NS '12:34:56.123456789' returns Nanos = 452961234567, 100 times too small

About the function tests: a C# function can't return TIME_NS once this type is gone, but that's the same today for TIMESTAMP_S, TIMESTAMP_MS, TIMESTAMP_NS and UHUGEINT. Each .NET type maps to one DuckDB type. Results in those types need a way to name the DuckDB type of a result, which is a separate feature for all of them, not part of this PR.

DuckDBTimeNs.FromTicks in Bindings stays: the writer and the converter use it.

@Giorgi

Giorgi commented Nov 5, 2026

Copy link
Copy Markdown
Owner

@Mr444-dev Do you plan to address these comments?

@Mr444-dev

Copy link
Copy Markdown
Contributor Author

Hi Giorgi, yes — I’ve addressed the outstanding points in commit d647f8e:

  • Moved the TIME_NS write tests next to the TIME tests and kept the appender comparison as a join against VALUES.
  • Removed checked from DuckDBTimeNs.FromTicks.
  • Removed DuckDBTimeNs as a public Data-layer input/result/type mapping, while keeping the native layout and FromTicks in Bindings. I also removed the direct raw-type appender, parameter, scalar-function, and table-function tests.
  • Updated the PR description to reflect the final scope.

Validation: the focused TIME_NS write tests passed 5/5 on five consecutive runs; the standard suite passed 1,691 tests with 1 skipped; the Data Release build succeeded with 0 warnings and 0 errors; git diff --check passed.

Thanks for the review. Could you please take another look?

@Giorgi
Giorgi merged commit c81f04c into Giorgi:develop Nov 5, 2026
11 of 12 checks passed

This branch is waiting to be deployed

1 waiting deployment
external — d647f8ec Waiting Nov 5, 2026 by Mr444-dev via Authorize #733
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.

3 participants