Parquet TIME now decodes to Time64, which Parquet/ORC/Avro writers reject

Open
#121,051 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
52/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
cpp
Domain
databases

Research direction

Start with the reproducer using tests/queries/0_stateless/data_parquet/time64_test.parquet, then inspect SchemaConverter.cpp:1237 and the writer paths in PrepareForWrite.cpp:530, ORCBlockOutputFormat.cpp:303, and AvroRowOutputFormat.cpp:512. Verify the TIME, TIME_MILLIS, and TIME_MICROS round trips for Parquet, ORC, and Avro, and resolve the documented ORC representation decision before treating the issue as done.

Written by the indexing model from the issue text.

Description

comp-formats
Describe what's wrong
  • Reading a Parquet file with a TIME column and inserting it back into Parquet fails with UNKNOWN_TYPE
  • Trigger is any insert into a file function with Parquet, ORC or Avro output selecting a TIME, TIME_MILLIS or TIME_MICROS column
  • The round trip used to work and should still write the column, but all three writers now reject Time64
  • ORC and Avro output fail with ILLEGAL_COLUMN, so ETL pipelines break on upgrade with default settings

Root cause: SchemaConverter::processPrimitiveColumn now infers DataTypeTime64(scale) instead of DataTypeDateTime64(scale, tz) for parquet TIME logical types. That type reaches Parquet::preparePrimitiveColumn, whose TypeIndex switch has no Time/Time64 case and falls through to the default throw.

Analysis details (evidence, affected locations, impact)

Why we believe this is a bug: SchemaConverter::processPrimitiveColumn (src/Processors/Formats/Impl/Parquet/SchemaConverter.cpp:1237) now infers DataTypeTime64(scale) for logical.TIME/TIME_MILLIS/TIME_MICROS instead of DataTypeDateTime64(scale, tz). That type reaches the output side unchanged, and Parquet::preparePrimitiveColumn (PrepareForWrite.cpp:530) has no TypeIndex::Time/TypeIndex::Time64 case, so it falls into default: and throws.

Affected locations:

Impact: Any ETL that reads a Parquet file containing a TIME/TIME_MILLIS/TIME_MICROS column and writes Parquet, ORC or Avro breaks after upgrade, with default settings and no way to work around it other than adding an explicit DateTime64 type hint to every such column. Before the PR the column decoded as DateTime64, which all three writers support.

Does it reproduce on most recent release?

Yes — confirmed on current master (commit f9fa351aada28).

How to reproduce

Uses tests/queries/0_stateless/data_parquet/time64_test.parquet with engine_file_truncate_on_insert = 1

# A Parquet file holding a TIME column must still be writable back to Parquet.

IN=test_repro.in.parquet
OUT=test_repro.out.parquet
cp "$CURDIR"/data_parquet/time64_test.parquet "$IN"

clickhouse-local -q "
    INSERT INTO FUNCTION file('$OUT', Parquet) SELECT * FROM file('$IN', Parquet)
        SETTINGS engine_file_truncate_on_insert = 1;
    SELECT * FROM file('$OUT', Parquet) ORDER BY ALL;
"

rm -f "$IN" "$OUT"

Note: an automated re-run of this exact block on current master (f9fa351aada2) did not show the failure; the analyst's run did (outputs below). Environment or ordering may matter.

Expected behavior

The TIME column should round trip and the output should contain the three time values

Expected output of the reproducer above:

00:00:20.000000
00:50:00.000000
01:23:20.000000
Error message and/or stacktrace

The insert throws UNKNOWN_TYPE saying the internal type Time64 of column timestamp is not supported for conversion into Parquet

Actual output of the reproducer above on master (f9fa351aada28):

[1 / 1] 05230_parquet_time_reexport:                                            [ FAIL ] 0.34 sec.
Reason: having stderror:
Code: 50. DB::Exception: Internal type 'Time64' of column 'timestamp' is not supported for conversion into Parquet data format. (UNKNOWN_TYPE)

stdout:
Suggested fix

Add case TypeIndex::Time64: (and TypeIndex::Time) to preparePrimitiveColumn in src/Processors/Formats/Impl/Parquet/PrepareForWrite.cpp, emitting parq::TimeType with isAdjustedToUTC=false and MILLIS/MICROS/NANOS per scale - the exact inverse of the new reader branch, so the round-trip is lossless. The ORC and Avro writers need the equivalent (ORC has no TIME type, so Time64 would have to map to its nearest representation or stay unsupported with a clearer message).

Additional context

Same pattern as #120653 (found by: lexical, vector, fix_path; vector: cosine distance 0.28 (agrees with the lexical leg)).

Open risks:

  • ORC has no time-of-day logical type, so a lossless ORC mapping may not exist; a decision is needed on whether Time64 should be written as an integer or keep throwing.

Found during automated review of PR #106019; whether that PR introduced it could not be established, so nobody is tagged. Severity P1 · Finding h_pr106019_001

Dominant language
C++
Stars
50k
Forks
9k
Avg merge
18h 29m
Merged PRs (30d)
511

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from ClickHouse/ClickHouse

All issues in ClickHouse/ClickHouse

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.