lxy-9602 commented on code in PR #478: URL: https://github.com/apache/paimon-cpp/pull/478#discussion_r4397190218
########## docs/source/user_guide/data_types.rst: ########## @@ -146,13 +146,24 @@ and `Arrow DataTypes <https://arrow.apache.org/docs/format/Columnar.html#data-ty * - ``TIME`` ``TIME(p)`` - - Not Supported + - Time32 - Data type of a time without time zone consisting of hour:minute:second[.fractional] with up to nanosecond precision and values ranging from 00:00:00.000000000 to 23:59:59.999999999. The type can be declared using ``TIME(p)`` where p is the number of digits of fractional seconds (precision). p must have a value between 0 and 9 (both inclusive). If no precision is specified, p is equal to 0. + Paimon C++ maps every ``TIME(p)`` to an Arrow ``Time32`` in milliseconds + (``time32[ms]``) and accepts no other Arrow time type. The declared + precision p is kept in the ``paimon.time.precision`` field metadata; a + ``Time32`` field without it is ``TIME(0)``. As in Paimon Java, values are + stored as milliseconds of the day whatever p is, so ``TIME(6)`` and + ``TIME(9)`` values keep only millisecond precision. + + Paimon C++ reads TIME columns from Parquet, Lance and Vortex data files + and writes them to Lance and Vortex data files. ORC and Avro data files + do not support TIME, and a TIME column cannot be a partition key. + Review Comment: Currently, the `time` type isn’t supported for PK tables, regardless of whether it’s used as a primary key. That’s why we didn’t include `time` type support in the documentation earlier. If we want to add it, please at least describe the limitations of the `time` type clearly. ########## test/test_data/lance/append_java_compat.db/append_java_compat/README.md: ########## @@ -54,5 +54,6 @@ Commit - snapshot 2 (4 rows total) At this Java revision, nullable ROW declarations are accepted but actual null ROW parents do not survive the Lance round trip (a separate probe read id=2's null parent as non-null). Both struct parents here are valid, including id=2 where their children are NULL. -TIME32 is omitted because the C++ table path does not support it; Java's Lance validator -rejects local-zoned timestamps, MAP, MULTISET, VARIANT and BLOB. +TIME32 is omitted because the C++ table path did not support it when this fixture was +written. At this revision, Java's Lance validator rejects local-zoned timestamps, MAP, +MULTISET, VARIANT and BLOB. Review Comment: Putting the “At this revision” description in the README doesn’t feel quite well—it’s hard to tell which revision that refers to. Also, if Lance supports time types, please update/override `append_java_compat` and `append_python_compat` to rewrite the test data, adding a `time` field to ensure Java/Python written outputs remain compatible. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
