Repository: duckdb/duckdb (language: python)

Issue:
DuckDB does not properly read TIMETZ types
### What happens?

The attached parquet file as a single TIMETZ column and DuckDB can not properly parse the timezone value. 
The parquet file viewed via https://dataconverter.io/view/parquet looks like this:
![image](https://github.com/user-attachments/assets/735cf489-4da8-4340-bfcf-1b9121501d17)

However, DuckDB returns the following:
```
D select * from 'timetz_column.parquet';
┌─────────────────────┐
│        col33        │
│ time with time zone │
├─────────────────────┤
│ at:33:32.627776+00  │
│ iv:26:11.627776+00  │
│ IL:29:15.627776+00  │
└─────────────────────┘
```
[timetz_column.parquet.zip](https://github.com/user-attachments/files/17651418/timetz_column.parquet.zip)

### To Reproduce

Download the attached parquet file. With DuckDB cli, run the following:
```
duckdb
D select * from 'timetz_column.parquet';
```

### OS:

MacOS M1 

### DuckDB Version:

1.1.3 (and older versions)

### DuckDB Client:

CLI

### Hardware:

_No response_

### Full Name:

Justin Tomlin

### Affiliation:

Alteryx

### What is the latest build you tested with? If possible, we recommend testing with the latest nightly build.

I have tested with a stable release

### Did you include all relevant data sets for reproducing the issue?

Yes

### Did you include all code required to reproduce the issue?

- [X] Yes, I have

### Did you include all relevant configuration (e.g., CPU architecture, Python version, Linux distribution) to reproduce the issue?

- [X] Yes, I have


Discussion / hints:
Is TIMESTAMPTZ also affected?
The main branch acts different slightly. I'll check TIMESTAMPZ.

```
% ~/ws/duckdb/build/duckdb
v1.1.4-dev1594 0ccf3c25cc
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
D select * from 'timetz_column.parquet';
┌──────────────────────┐
│        col33         │
│ time with time zone  │
├──────────────────────┤
│ \0\0:33:32.627776+00 │
│ ????INVALID VALUE …  │
│ ????INVALID VALUE …  │
└──────────────────────┘
D 
```
> Is `TIMESTAMPTZ` also affected?

@BigBerny there is no time zone embedded in parquet timestamps (or in SQL `TIMESTAMPTZ` types), and this path is well tested, so the answer here is almost certainly no.
 
`TIMETZ` is another matter, and I have been unable to find any documentation on how `isAdjustedToUTC` affects `TIME` types. The data in the file provided is outside the range of valid time values `[00:00:00-24:00:00]`:

```
301:33:32.627776    &raw_micros	const int64_t	1085612627776
290:26:11.627776    &raw_micros	const int64_t	1045571627776
294:29:15.627776    &raw_micros	const int64_t	1060155627776
```
In debug, the code throws because the hour is out of range, so I suspect the garbage display is caused by the formatter assuming that the data was validated on input. Unfortunately, it appears that the parquet reader is not validating the input properly. 

Right now it assumes that the time is a count of µs since the epoch and set the offset to 0, but that may not be correct. I can probably fix this @jtomlin12 if you can tell me how the file was created or point me at how parquet represents the offsets?

I did notice that the hours are correct if they are taken mod 24, and the div is 12 for all three values, so it looks like the times have been offset by 12 hours. I have never seen this representation before, it does not appear to be documented anywhere and I am loath to assume that offsets are encoded in this way (minutes?, seconds? negative hours west of Greenwich?)
Sorry I am a bit confused... the offset represents 12 _days_. Was this originally a time delta field?
Great findings @hawkfish, thanks for looking at this! 

I extracted col33 from a larger dataset using using DuckDB cli to create the `timetz_column.parquet`. This may have had some affect on the data. For reference, the initial dataset starts out in a postgres DB which I used two different SQL tools to query col33 (Apache Drill and PSQL):

![image](https://github.com/user-attachments/assets/65f193ca-04cc-457c-82a5-b506e41e30f9)

The pqsl output on the right shows the timezones but they are technically not valid. The left (apache output) seems to be adjusted based on the offsets. I used ApacheDrill to output this data to a parquet file: [timetz_column_generate_by_apache_drill.parquet.zip](https://github.com/user-attachments/files/17670792/timetz_column_generate_by_apache_drill.parquet.zip)

This file CAN be parsed by duckdb:
```
D select * from '~/Downloads/timetz_column_generate_by_apache_drill.parquet';
┌─────────────────────┐
│        col33        │
│ time with time zone │
├─────────────────────┤
│ 15:08:21+00         │
│ 04:01:00+00         │
│ 08:04:04+00         │
└─────────────────────┘
```
Internally, we query from postgres and use `ParquetWriter` from https://github.com/apache/parquet-java to produce the parquet file that DuckDB fails to read which is also apache. After the data gets through our system the `part-0.parquet` file is produced:

![image](https://github.com/user-attachments/assets/264962fe-9f85-44a2-9b4e-2d015cdf7203)

As you can see, the `timetz_column.parquet` file is certainly corrupted because `13:33:32.6227` doesn't look anything like the initial dataset. Apache Drill _can_ parse the `part-0.parquet` and these time values are adjusted to GMT which matches how other parts of our system display this data. Ultimately this is what we expected from DuckDB as well.


I'm confused by this:

> The pqsl output on the right shows the timezones but they are technically not valid.

Why are they not valid? PG supports `TIMETZ` values with offsets ±15:59:59 (the docs leave off the seconds, but I have verified this empirically). It looks more to me like the values in the PG test table were not inserted correctly?

Incidentally, the `TIME WITH TIME ZONE` type has an even worse name than  `TIMESTAMP WITH TIME ZONE`: it doesn't model time zones at all (how could it without a date?) it just stores an offset and then has some rather confusing comparison semantics bolted on top of that.


According to the Drill internal comments, their TTZ type is not the same as SQLs:

```java
    /**
     * <pre>
     *  time in micros before or after 2000/1/1 with timezone
     * </pre>
     *
     * <code>TIMETZ = 14;</code>
     */
    TIMETZ(14),
```

(see `common/src/main/java/org/apache/drill/common/types/Types.java`)
I'm still not clear what steps you went through to generate this (invalid?) parquet file. 

- You started with a PG table that has a bunch of random? values in columns. `col33` is a PG `TIMETZ` column with values displayed above by Trifacta? (You said `psql` but the prompt says `trifacta`. Frankly I would not trust _any_ tool to display PG `TIMETZ` columns correctly except `psql`) 
- You used the duckdb Postgres connector to read the column and write it out to parquet?
- You also used Apache Drill to connect to Postgres (using the PG driver?) and used it to write out another parquet file?
FYI, `parquet-tools dump` considers that the values are overflowed.

```
% java -jar ~/Downloads/parquet-tools-1.11.0.jar dump --debug ./timetz_column.parquet
row group 0 
--------------------------------------------------------------------------------
col33:  INT64 SNAPPY DO:0 FPO:4 SZ:49/47/0.96 VC:3 ENC:PLAIN ST:[min:  [more]...

    col33 TV=3 RL=0 DL=1
    ----------------------------------------------------------------------------
    page 0:  DLE:RLE RLE:RLE VLE:PLAIN ST:[no stats for this column] CRC:[none] [more]...

INT64 col33 
--------------------------------------------------------------------------------
*** row group 1 of 1, values 1 to 3 *** 
value 1: R:0 D:1 V:301:33:32.627776+0000
value 2: R:0 D:1 V:290:26:11.627776+0000
value 3: R:0 D:1 V:294:29:15.627776+0000
```
It sounds like the original data did not have those strange fractional seconds and were in range. So I think the wire data from PG included the offsets, but whatever tool read them did not know how to separate the offsets and time data. It did sound like the tool was the DuckDB PG extension? If so, that is likely the source of the problem. This types is not well tested because it is obscure and rarely used, so that may well be where the corruption snuck in.
Again thanks for the responses! @hawkfish It was wrong to say they were valid, I was thinking the offsets just didn't match UTC time offsets like (+5:00, etc.) so I can accept them now as just being general offsets. 

To clarify, 
* `trifacta` is the name of the database and psql seems to set that as the prompt of the psql terminal.
* I was actually unaware of the DuckDB's PostgresSQL extension until you said it. Using the extension, I queried the dataset from postgres and it returns the correct data from postgres (so the extension works):

```
D select col33 from psdb.test_schema.allTypes_Test;
┌─────────────────────┐
│        col33        │
│ time with time zone │
├─────────────────────┤
│ 10:07:21+13:59      │
│ 00:00:00+14:59      │
│ 02:03:04+12:59      │
└─────────────────────┘
```
* I created `timetz_column.parquet` using DuckDB cli from the full dataset here: [part-0.parquet.zip](https://github.com/user-attachments/files/17680621/part-0.parquet.zip).

Viewing the `part-0.parquet` (which was generated by Java code with Apache API) with the online parquet viewer, I see the following:
![image](https://github.com/user-attachments/assets/56f9b08d-dfbc-4fb8-8c91-8d3714180576)
This seems to be correctly adjusted to GMT but duckdb still can't read it:
```
D select col33 from 'part-0.parquet';
┌─────────────────────┐
│        col33        │
│ time with time zone │
├─────────────────────┤
│ mt:33:32.627776+00  │
│ at:26:11.627776+00  │
│ EE:29:15.627776+00  │
└─────────────────────┘
```
Hope this clarifies some things, sorry for the confusion. Should we expect DuckDB to also adjust to GMT? 

Thanks, some things are becoming clearer. So the original data has the fractional offsets and this path works:

PG => PG extension => duckdb => CLI output

I'm still not clear how `part-0.parquet` is produced? Above you say

> After the data gets through our system the `part-0.parquet` file is produced:

so it sounds like some other tool is producing this parquet file? You also say

> Viewing the `part-0.parquet` (which was generated by Java code with Apache API) with the online parquet viewer, I see the following:

which suggests that your system is using a Java tool of some kind to produce this file?  If this is the case, then I think the problem is in that tool. 

I'm also not clear what is meant by 

> This seems to be correctly adjusted to GMT but duckdb still can't read it

No adjusting to GMT is done with `TTZ` types except for sorting. I can see that e.g., `10:07:21+13:59` is equivalent to `20:08:21` (if you wrap) but that is not the same data type (Is it `TIME`?) and the two values are not the same in the SQL definition.

Unless I have misunderstood something, it sounds to me like the Java tool does not handle this type correctly (or at least for SQL). It certainly sounds like something outside of DuckDB is producing corrupt `TIME` columns with the `isAdjustedToUTC` flag set. So for us, there are two issues:

- If there is a path to producing a corrupt parquet column using _only_ DuckDB, then that is a bug. (Note that you can insert into a table using the values as strings and bypass the PG connector)
- If it is possible for outside tools to create corrupt columns of this type, then we should probably either wrap them or throw an error.
The following test script works as expected:

```sql
require parquet

statement ok
CREATE TABLE timetz_column (col33 TIME WITH TIME ZONE);

statement ok
INSERT INTO timetz_column VALUES 
	('10:07:21+13:59'),
	('00:00:00+14:59'),
	('02:03:04+12:59'),
;

query I
SELECT * FROM timetz_column;
----
10:07:21+13:59
00:00:00+14:59
02:03:04+12:59

statement ok
COPY (FROM timetz_column) TO 'timetz_column.parquet';

# Note that the offset has been stripped 
# because parquet cannot represent it,
# and normalising would wrap.
query I
SELECT * FROM 'timetz_column.parquet';
----
10:07:21+00
00:00:00+00
02:03:04+00
```
Great feedback @hawkfish thank you! I'll dig deeper into that Java tool and hopefully find the root cause. 
I have new insights. Our java code parses postgres data and generates the parquet sets `isAdjustedToUTC` as true by default and these third-party libraries: 
* "org.apache.parquet:parquet-avro:1.14.1",
* "org.apache.parquet:parquet-hadoop:1.14.1"

The `TIME_WITH_TIMEZONE` value of col33 in postgres (`10:07:21+13:59`) gets extracted and converted to `-13899000` milliseconds since midnight. Here's that calculation: 
* Local time in milliseconds: `10 * 3600 * 1000 + 7 * 60 * 1000 + 21 * 1000 = 36441000` milliseconds
* Time zone offset: `+13:59` (which is `13 * 3600 + 59 * 60 = 50340` seconds or `50340000` milliseconds)
* Adjusted milliseconds: `36441000 - 50340000 = -13899000` milliseconds

@hawkfish Does that seem valid? Can DuckDB handle negative timetz values? 
The math makes sense I suppose, but the information for the offset has been lost so there is not a lot we can do with it. Basically, we could produce a time+offset starting with 00:00:00+13899000ms but there are a large number of legal possibilities. We should throw, though, as the values are not valid SQL `TIMETZ` values.
I think what we could do here is wrap all values to [00:00:00, 24:00:00).
I agree in a sense but perhaps this check should also be done elsewhere (outside of DuckDB), such as before it's been written to parquet. From what I found today, I think there _is_ an expectation that the data being written to parquet should be non-negative given that this type _should_ represent a "time-of-day" value. However I couldn't find any documentation supporting this other than AI answers. In our case this could be an issue better suited for: https://github.com/apache/parquet-java. At the moment, my team might consider invalidating this data on the Java side, before it becomes a parquet. Thanks for the info and efforts getting to this point! @hawkfish 
The microsecond values in the parquet file you originally provided are not negative:

```
301:33:32.627776    &raw_micros	const int64_t	1085612627776
290:26:11.627776    &raw_micros	const int64_t	1045571627776
294:29:15.627776    &raw_micros	const int64_t	1060155627776
```
Can you check this file? Also what tool are you using to read that?
[part-0.parquet.zip](https://github.com/user-attachments/files/17821535/part-0.parquet.zip)

@jtomlin12 Our parquet reader still needs to act defensively to handle out of range values. I will add wrapping logic to make sure. The alternative is to throw, but that seems aggressive for now.
Do you have a parquet file with negative times?
> Do you have a parquet file with negative times?

Pretty sure `part-0.parquet` has a negative time value in col33. 

Thanks. With wrapping we now handle that file the same way as Drill. These are the results from all three files:

```sql
require parquet

query I
select col33 
from '~/Downloads/timetz_column.parquet';
----
13:33:32.627776+00
02:26:11.627776+00
06:29:15.627776+00

query I
select col33 
from '~/Downloads/part-0.parquet';
----
20:08:21+00
09:01:00+00
13:04:04+00

query I
select col33 
from 'timetz_column.parquet';
----
10:07:21+00
00:00:00+00
02:03:04+00
``` 
I'm going to throw `part-0.parquet` into the test suite unless you object?
> I'm going to throw `part-0.parquet` into the test suite unless you object?

No, be my guest. Thank you!

Fix patch:
diff --git a/extension/parquet/parquet_timestamp.cpp b/extension/parquet/parquet_timestamp.cpp
index a0ada7d14e70..84c158f1956d 100644
--- a/extension/parquet/parquet_timestamp.cpp
+++ b/extension/parquet/parquet_timestamp.cpp
@@ -105,6 +105,16 @@ date_t ParquetIntToDate(const int32_t &raw_date) {
 	return date_t(raw_date);
 }
 
+template <typename T>
+static T ParquetWrapTime(const T &raw, const T day) {
+	// Special case 24:00:00
+	if (raw == day) {
+		return raw;
+	}
+	const auto modulus = raw % day;
+	return modulus + (modulus < 0) * day;
+}
+
 dtime_t ParquetIntToTimeMs(const int32_t &raw_millis) {
 	return Time::FromTimeMs(raw_millis);
 }
@@ -118,15 +128,19 @@ dtime_t ParquetIntToTimeNs(const int64_t &raw_nanos) {
 }
 
 dtime_tz_t ParquetIntToTimeMsTZ(const int32_t &raw_millis) {
-	return dtime_tz_t(Time::FromTimeMs(raw_millis), 0);
+	const int32_t MSECS_PER_DAY = Interval::MSECS_PER_SEC * Interval::SECS_PER_DAY;
+	const auto millis = ParquetWrapTime(raw_millis, MSECS_PER_DAY);
+	return dtime_tz_t(Time::FromTimeMs(millis), 0);
 }
 
 dtime_tz_t ParquetIntToTimeTZ(const int64_t &raw_micros) {
-	return dtime_tz_t(dtime_t(raw_micros), 0);
+	const auto micros = ParquetWrapTime(raw_micros, Interval::MICROS_PER_DAY);
+	return dtime_tz_t(dtime_t(micros), 0);
 }
 
 dtime_tz_t ParquetIntToTimeNsTZ(const int64_t &raw_nanos) {
-	return dtime_tz_t(Time::FromTimeNs(raw_nanos), 0);
+	const auto nanos = ParquetWrapTime(raw_nanos, Interval::NANOS_PER_DAY);
+	return dtime_tz_t(Time::FromTimeNs(nanos), 0);
 }
 
 } // namespace duckdb


Test patch:
diff --git a/data/parquet-testing/negative-timetz.parquet b/data/parquet-testing/negative-timetz.parquet
new file mode 100644
index 000000000000..f09ee5342f39
Binary files /dev/null and b/data/parquet-testing/negative-timetz.parquet differ
diff --git a/test/parquet/timetz_parquet.test b/test/parquet/timetz_parquet.test
index 909148944585..84290efda703 100644
--- a/test/parquet/timetz_parquet.test
+++ b/test/parquet/timetz_parquet.test
@@ -36,3 +36,12 @@ where pruefbahn_id = '58981';
 58981	07:20:00+00
 58981	07:20:00+00
 58981	07:20:00+00
+
+query I
+select col33 
+from 'data/parquet-testing/negative-timetz.parquet';
+----
+20:08:21+00
+09:01:00+00
+13:04:04+00
+