Repository navigation
feat(data): support writing TIME_NS values - #387
Conversation
Coverage Report for CI Build 42059113838Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.6%) to 93.39%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions52 previously-covered lines in 14 files lost coverage.
Coverage Stats💛 - Coveralls |
|
Thanks for picking up the write side. Please change these three things: 1. The mapped appender still refuses TIME_NS. 2. Compare with DuckDB, not with constants worked out in C#. The header of 3. |
|
One more thing: the write tests cover only part of
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. |
|
One more: please keep the TIME_NS conversion in one place for both directions. The read side already has it on Please add the inverse of |
|
@Mr444-dev Can you make these changes? 👆 |
|
I addressed the requested changes in commit
The focused |
|
Thanks, most of the requested changes are in. A few smaller things:
|
|
Please remove the Please take out:
As it stands, the support is also incomplete:
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.
|
|
@Mr444-dev Do you plan to address these comments? |
|
Hi Giorgi, yes — I’ve addressed the outstanding points in commit
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; Thanks for the review. Could you please take another look? |
Summary
Validation