Skip to content

ORC: Disable predicate pushdown for timestamp type #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdblue merged 2 commits into from
May 26, 2020
Merged

ORC: Disable predicate pushdown for timestamp type #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed91355
Correctly handle timestamps less than epoch
shardulm94 May 26, 2020
63bc62b
Disable ORC pushdown for timestamp types because of ORC-611
shardulm94 May 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Types;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -457,7 +456,6 @@ private DataFile writeFile(String location, String filename, Schema schema, List

@Test
public void testFilterWithDateAndTimestamp() throws IOException {
Assume.assumeFalse(format == FileFormat.ORC);
Schema schema = new Schema(
required(1, "timestamp_with_zone", Types.TimestampType.withZone()),
required(2, "timestamp_without_zone", Types.TimestampType.withoutZone()),
Expand Down
13 changes: 7 additions & 6 deletions data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilterTypes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,13 @@ public static Object[][] parameters() {
new Object[] { "orc", "double", 2.11d, 1.97d },
new Object[] { "orc", "date", "2018-06-29", "2018-05-03" },
new Object[] { "orc", "time", "10:02:34.000000", "10:02:34.000001" },
new Object[] { "orc", "timestamp",
"2018-06-29T10:02:34.000000",
"2018-06-29T15:02:34.000000" },
new Object[] { "orc", "timestamptz",
"2018-06-29T10:02:34.000000+00:00",
"2018-06-29T10:02:34.000000-07:00" },
// Temporarily disable filters on Timestamp columns due to ORC-611
// new Object[] { "orc", "timestamp",
// "2018-06-29T10:02:34.000000",
// "2018-06-29T15:02:34.000000" },
// new Object[] { "orc", "timestamptz",
// "2018-06-29T10:02:34.000000+00:00",
// "2018-06-29T10:02:34.000000-07:00" },
new Object[] { "orc", "string", "tapir", "monthly" },
// uuid, fixed and binary types not supported yet
// new Object[] { "orc", "uuid", uuid, UUID.randomUUID() },
Expand Down
7 changes: 5 additions & 2 deletions orc/src/main/java/org/apache/iceberg/orc/ExpressionToSearchArgument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ static SearchArgument convert(Expression expr, TypeDescription readSchema) {

// Currently every predicate in ORC requires a PredicateLeaf.Type field which is not available for these Iceberg types
private static final Set<TypeID> UNSUPPORTED_TYPES = ImmutableSet.of(
TypeID.TIMESTAMP, // Temporarily disable filters on Timestamp columns due to ORC-611
TypeID.BINARY,
TypeID.FIXED,
TypeID.UUID,
Expand Down Expand Up @@ -254,8 +255,10 @@ private <T> Object literal(Type icebergType, T icebergLiteral) {
return Date.valueOf(LocalDate.ofEpochDay((Integer) icebergLiteral));
case TIMESTAMP:
long microsFromEpoch = (Long) icebergLiteral;
return Timestamp.from(Instant.ofEpochSecond(Math.floorDiv(microsFromEpoch, 1_000_000),
(microsFromEpoch % 1_000_000) * 1_000));
return Timestamp.from(Instant.ofEpochSecond(
Math.floorDiv(microsFromEpoch, 1_000_000),
Math.floorMod(microsFromEpoch, 1_000_000) * 1_000
Copy link
Contributor

@rdblue rdblue May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

@shardulm94 shardulm94 May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous logic did not correctly handle pre-epoch timestamps and resulted in off by one second error which was the primary cause of failures in #983 (% returns negative values if sign(a)!=sign(b) so it interfered with floorDiv which also has handling for the same). After I fixed this issue, I ran into ORC-611,

));
case DECIMAL:
return new HiveDecimalWritable(HiveDecimal.create((BigDecimal) icebergLiteral, false));
default:
Expand Down
17 changes: 12 additions & 5 deletions orc/src/test/java/org/apache/iceberg/orc/TestExpressionToSearchArgument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@
import java.math.BigDecimal;
import java.nio.ByteBuffer;
import java.sql.Date;
import java.sql.Timestamp;
import java.time.Instant;
import java.time.LocalDate;
import java.time.OffsetDateTime;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;
import java.util.TimeZone;
import java.util.UUID;
import org.apache.iceberg.Schema;
Expand Down Expand Up @@ -105,22 +107,27 @@ public void testPrimitiveTypes() {
public void testTimezoneSensitiveTypes() {
for (String timezone : new String[]{"America/New_York", "Asia/Kolkata", "UTC/Greenwich"}) {
TimeZone.setDefault(TimeZone.getTimeZone(timezone));
OffsetDateTime tsTzPredicate = OffsetDateTime.parse("2019-10-02T00:47:28.207366Z");
OffsetDateTime tsPredicate = OffsetDateTime.parse("1968-01-16T13:07:59.048625Z");
OffsetDateTime epoch = Instant.ofEpochSecond(0).atOffset(ZoneOffset.UTC);

Schema schema = new Schema(
required(1, "date", Types.DateType.get()),
required(2, "tsTz", Types.TimestampType.withZone()),
required(3, "ts", Types.TimestampType.withoutZone())
);

Expression expr = and(
and(equal("date", 10L), equal("tsTz", 10 * 3600 * 1000000L)),
equal("ts", 20 * 3600 * 1000000L)
and(equal("date", 10L), equal("tsTz", ChronoUnit.MICROS.between(epoch, tsTzPredicate))),
equal("ts", ChronoUnit.MICROS.between(epoch, tsPredicate))
);
Expression boundFilter = Binder.bind(schema.asStruct(), expr, true);
SearchArgument expected = SearchArgumentFactory.newBuilder()
.startAnd()
.equals("`date`", Type.DATE, Date.valueOf(LocalDate.parse("1970-01-11", DateTimeFormatter.ISO_LOCAL_DATE)))
.equals("`tsTz`", Type.TIMESTAMP, Timestamp.from(Instant.ofEpochSecond(10 * 3600)))
.equals("`ts`", Type.TIMESTAMP, Timestamp.from(Instant.ofEpochSecond(20 * 3600)))
// Temporarily disable filters on Timestamp columns due to ORC-611
// .equals("`tsTz`", Type.TIMESTAMP, Timestamp.from(tsTzPredicate.toInstant()))
// .equals("`ts`", Type.TIMESTAMP, Timestamp.from(tsPredicate.toInstant()))
.end()
.build();

Expand Down