Complete iceberg support for time type #24091

auden-woolfson · 2024-11-19T21:56:44Z

Description

Bringing this functionality from IBM's repository. Added support for partitioning over the time type in orc files through the iceberg connector

Lakehouse PR for those with access

== RELEASE NOTES ==

General Changes
* Add support for time type partitioning in the ORC file format for Iceberg. :pr:`24091`
* Add testing for partitioning using time type in Iceberg. :pr:`24091`

sdruzkin

ORC changes look good.

sdruzkin · 2024-11-23T21:19:46Z

presto-orc/src/main/java/com/facebook/presto/orc/reader/LongDirectBatchStreamReader.java

@@ -186,6 +196,8 @@ private Block readNullBlock(boolean[] isNull, int nonNullCount)
        throw new VerifyError("Unsupported type " + type);
    }

+    protected void maybeTransformValues(long[] values, int nextBatchSize) {}


Why do we need this?

Actually I don't think we do here... just took this directly from the IBM code. Will push version without it

ZacBlanco

minor change, otherwise looks good

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

ScrapCodes

Thanks for working on this, would be great if we can improve test coverage.

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

ScrapCodes · 2024-12-04T06:05:17Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

+        assertQuery(format("SELECT COUNT(*) FROM %s", tableName), "SELECT 2");
+        assertQuery(format("SELECT x FROM %s WHERE y = 12345", tableName), "SELECT CAST('10:12:34' AS TIME)");
+        assertQuery(format("SELECT x FROM %s WHERE y = 67890", tableName), "SELECT CAST('9:00:00' AS TIME)");
+        dropTable(session, tableName);


Can we include groupby and orderby as well

hantangwangd

I believe we have to handle the write logic for TimeType on ORC as well. The raw data in ORC file should follow Iceberg's specification (in micros precision), it's a little different from the memory format in presto (in millis precision).

hantangwangd · 2024-12-06T01:06:51Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/TypeConverter.java

@@ -310,6 +310,9 @@ private static TypeInfo toHiveTypeInfo(Type type)
        if (DOUBLE.equals(type)) {
            return HIVE_DOUBLE.getTypeInfo();
        }
+        if (TimeType.TIME.equals(type)) {
+            return HIVE_INT.getTypeInfo();


Should it be HIVE_LONG?

Why would it be long? This is how it was merged internally...

Time is defined to be in microseconds in Iceberg, so long can make sense. Even if time can only represent between 00:00 and 23:59.99999. There are 86,400 seconds in a day; so 86.4B microseconds which exceeds integer size. So it should probably be a long.

hantangwangd · 2024-12-06T01:12:00Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

+        assertQuery(format("SELECT x FROM %s WHERE y = 12345", tableName), "SELECT CAST('10:12:34' AS TIME)");
+        assertQuery(format("SELECT x FROM %s WHERE y = 67890", tableName), "SELECT CAST('9:00:00' AS TIME)");


Seems when executing filter by a column with TimeType on ORC, we will meet some problems, no matter the TimeType column is a partitioned column or non-partitioned column.

For reference, see test cases in IcebergDistributedTestBase.testPartitionedByTimeType().

Not sure what you mean... that test is also passing. Is there something I am missing from the test cases that I should add?

The test IcebergDistributedTestBase.testPartitionedByTimeType() is currently running on PARQUET only, it includes the following scenarios that I think should also be added here to test on ORC:

assertQuery(format("SELECT y FROM %s WHERE x = time '10:12:34'", tableName), "values(12345)");

Could you please expand on how you identified this issue? Looked through some of the orc code and I'm not sure what I'm missing. Thank you

According to the Iceberg specification, the raw data for TimeType maintained in data files should be with microsecond precision. Meanwhile, the data for TimeType is maintained in presto with millisecond precision. So in order to support TimeType on PARQUET in PR #21337, we have done the following things:

Convert the data of type TimeType on writing, change the precision from milliseconds to microseconds. See: TimeValueWriter.

Convert the data of type TimeType on reading, change the precision from microseconds to milliseconds. See: LongTimeMicrosColumnReader.

Convert the TimeType values exist in filter predication. For Iceberg, filtering conditions need to be processed at two levels:

At the iceberg file plan level, see: IcebergSplitManager.getSplits(...) and ExpressionConverter.getIcebergLiteralValue().

At the iceberg page source level, the predicate passed to the newly constructed ConnectorPageSource should be handled (for PARQUET, the filter has been completely ignored since columnIndexFilterEnabled = false), see: IcebergPageSourceProvider.createParquetPageSource(...).

Handle the partitioned column with type TimeType, See IcebergPageSink.getIcebergValue(), IcebergPageSource.nativeValueToBlock(), and PartitionTable.convert().

So based on this context, in order to support TimeType on ORC, we need to do the following file format specific things:

Convert the data of type TimeType on writing, change the precision from milliseconds to microseconds in LongColumnWriter and LongDictionaryColumnWriter etc.

Convert the data of type TimeType on reading, change the precision from microseconds to milliseconds in LongDirectBatchStreamReader and LongDictionaryBatchStreamReader etc.

Convert the TimeType value in filter predication at the iceberg page source level, convert or ignore the predicates on columns of type TimeType in IcebergPageSourcePrivider.createBatchOrcPageSource().

In my local experiment, after doing all these things, the tests will all passing. You can have a try, and please let me know if there are any misunderstandings above.

Still a little unclear on how to go about step 3... do you have a branch that you made your local changes on?

Yes, you can refer to here: https://github.com/hantangwangd/presto/blob/support_timetype_on_orc/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSourceProvider.java#L576-L585. In this code, I simply skipped the filtering predicate on TimeType column.

Alright thanks. Was the error you got before making the fix the same as this?

java.lang.AssertionError: For query: SELECT y FROM test_selected_by_time WHERE x = time '10:12:34' actual column types: [bigint] expected column types: [bigint] not equal Actual rows (1 of 1 extra rows shown, 2 rows in total): [54321]

I made the changes you requested and I am still getting this error on testSelectOrPartitionedByTime

You have inserted another row, so the assertion statement should be:

assertQuery(format("SELECT y FROM %s WHERE x = time '10:12:34'", tableName), "values 12345, 54321");

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

steveburnett · 2025-01-07T18:43:47Z

Thanks for the release note entry! A couple of nits.

== RELEASE NOTES ==

General Changes
* Add support for time type partitioning in the ORC file format for Iceberg. :pr:`24091`
* Add testing for partitioning using time type in Iceberg. :pr:`24091`

ZacBlanco

minor nit, otherwise lgtm

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSourceProvider.java

auden-woolfson added the from:IBM PR from IBM label Nov 19, 2024

auden-woolfson requested review from sdruzkin, a team, hantangwangd and ZacBlanco as code owners November 19, 2024 21:56

auden-woolfson requested a review from presto-oss November 19, 2024 21:56

ethanyzhang requested review from a team and ScrapCodes and removed request for a team November 21, 2024 06:03

auden-woolfson force-pushed the oss_time_testing branch from d761fc0 to 717735c Compare November 21, 2024 21:08

sdruzkin previously approved these changes Nov 23, 2024

View reviewed changes

auden-woolfson dismissed sdruzkin’s stale review via ba62319 November 25, 2024 18:57

auden-woolfson force-pushed the oss_time_testing branch from ba62319 to f5e2b92 Compare November 27, 2024 22:43

ZacBlanco requested changes Dec 3, 2024

View reviewed changes

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Outdated Show resolved Hide resolved

ScrapCodes reviewed Dec 4, 2024

View reviewed changes

hantangwangd requested changes Dec 6, 2024

View reviewed changes

auden-woolfson force-pushed the oss_time_testing branch 4 times, most recently from aa9adb1 to 81a8a84 Compare December 13, 2024 22:21

hantangwangd requested changes Dec 18, 2024

View reviewed changes

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Outdated Show resolved Hide resolved

auden-woolfson force-pushed the oss_time_testing branch from 76302f2 to 90dd494 Compare January 7, 2025 22:02

ZacBlanco previously approved these changes Jan 7, 2025

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSourceProvider.java Outdated Show resolved Hide resolved

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSourceProvider.java Outdated Show resolved Hide resolved

auden-woolfson dismissed ZacBlanco’s stale review via e283d1a January 7, 2025 23:36

ZacBlanco approved these changes Jan 8, 2025

View reviewed changes

Add support for time type partitioning in ORC for iceberg with testing

77a97b0

auden-woolfson force-pushed the oss_time_testing branch from e283d1a to 77a97b0 Compare January 8, 2025 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete iceberg support for time type #24091

Complete iceberg support for time type #24091

auden-woolfson commented Nov 19, 2024 •

edited

Loading

sdruzkin left a comment

sdruzkin Nov 23, 2024

auden-woolfson Nov 25, 2024

ZacBlanco left a comment

ScrapCodes left a comment

ScrapCodes Dec 4, 2024

hantangwangd left a comment •

edited

Loading

hantangwangd Dec 6, 2024

auden-woolfson Dec 10, 2024

ZacBlanco Dec 10, 2024 •

edited

Loading

hantangwangd Dec 6, 2024

auden-woolfson Dec 10, 2024

hantangwangd Dec 11, 2024

auden-woolfson Dec 11, 2024

hantangwangd Dec 12, 2024

auden-woolfson Dec 16, 2024

hantangwangd Dec 17, 2024

auden-woolfson Dec 17, 2024

hantangwangd Dec 18, 2024

steveburnett commented Jan 7, 2025

ZacBlanco left a comment

		assertQuery(format("SELECT x FROM %s WHERE y = 12345", tableName), "SELECT CAST('10:12:34' AS TIME)");
		assertQuery(format("SELECT x FROM %s WHERE y = 67890", tableName), "SELECT CAST('9:00:00' AS TIME)");

Complete iceberg support for time type #24091

Are you sure you want to change the base?

Complete iceberg support for time type #24091

Conversation

auden-woolfson commented Nov 19, 2024 • edited Loading

Description

sdruzkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

ScrapCodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantangwangd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Jan 7, 2025

ZacBlanco left a comment

Choose a reason for hiding this comment

auden-woolfson commented Nov 19, 2024 •

edited

Loading

hantangwangd left a comment •

edited

Loading

ZacBlanco Dec 10, 2024 •

edited

Loading