Conversation
test/arrow/arrow_roundtrip.cpp
Outdated
| return; | ||
| #endif | ||
| TestArrowRoundtrip("SELECT * EXCLUDE(bit,time_tz, bignum) REPLACE " | ||
| TestArrowRoundtrip("SELECT * EXCLUDE(bit, time_ns, time_tz, bignum) REPLACE " |
There was a problem hiding this comment.
You need this patch to get time_ns to pass:
diff --git a/src/function/table/arrow/arrow_duck_schema.cpp b/src/function/table/arrow/arrow_duck_schema.cpp
index ab5611c13a..f722a5df09 100644
--- a/src/function/table/arrow/arrow_duck_schema.cpp
+++ b/src/function/table/arrow/arrow_duck_schema.cpp
@@ -370,6 +370,18 @@ LogicalType ArrowType::GetDuckType(bool use_dictionary) const {
}
return LogicalType::UNION(std::move(new_children));
}
+ case LogicalTypeId::TIME: {
+ auto time_info = type_info->Cast<ArrowDateTimeInfo>();
+ if (type.id() == LogicalTypeId::TIME) {
+ switch (time_info.GetDateTimeType()) {
+ case ArrowDateTimeType::NANOSECONDS:
+ return LogicalType::TIME_NS;
+ default:
+ return type;
+ }
+ }
+ return type;
+ }
default: {
if (extension_data) {
return extension_data->GetDuckDBType();I haven't tested whether this breaks anything else, but it shouldn't
If it does, we can leave it skipped for now
There was a problem hiding this comment.
Thanks for the tip - I found an even simpler change to this file (where the type for the "ttn" format is defined) that fixes this case. I was able to remove the time_ns exclusion, as well as all the other (existing) type exclusions, from the arrow_roundtrip test and have it pass!
|
It occurs to me that this change should have been submitted against In the meantime, I could use some help fixing the failing "Forwards compatibility tests". It looks like the tests are trying to load a database containing a table created with the latest |
|
Here's the |
|
It's probably best if the |
690b0a6 to
fc6c851
Compare
|
Based on the conversation in the |
fc6c851 to
cb73c6e
Compare
|
@pdet Could you have another look at this PR? I think we still want this for compatibility. There is a merge conflict due to the shell rework, but that should be easy to fix. |
|
Yeah, I'll pick it up! |
Picking up #19606, solving merge conflict, and removing unnecessary exclusions of `time_ns` type. We are still missing the tests for the different precision types of `TIME`, these should be added in our Python client after this code gets merged, as we need arrow to produce these precision types.
A
v1.4-andiumversion of this PR is here: #19613While attempting to add support for the relatively new TIME_NS logical type in Node Neo, I found some places where it was not fully supported. This is an attempt to support this type in more contexts. This doesn't cover everything, but should be a step forward.
Areas affected:
time_nscolumn from the tests for now. I welcome ideas of how to make this work, because it will likely be necessary for TIME_NS to work in DuckDB Wasm.test_all_typestime_nscolumn totest_all_typesrequired changing the expected values in several tests.I tried to update the parquet extension to support TIME_NS, but was unable to make it work. I've excluded the
time_nscolumn oftest_all_typesfrom the parquet tests that use it (following the pattern of several other unsupported types).There are likely other places that should support TIME_NS, such as various casts, several aggregate functions, and the CSV reader. I didn't attempt to update these areas.
FYI @hawkfish