You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:44:17 UTC
[GitHub] [incubator-iceberg] chenjunjiedada opened a new pull request #983: Convert date and timestamp values in generics
chenjunjiedada opened a new pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983
This fixes https://github.com/apache/incubator-iceberg/issues/972.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-632331000
@chenjunjiedada, I think what you have here works, but right now we have two sets of accessors (Spark and internal) and we would need more for other data models.
I think what we need to do instead is to separate the concerns. Accessors should understand structure and rows should return the correct value types. That way we can eventually move to use the same accessors and we will just need rows that correctly translate for StructLike.
How about using a wrapper class like this one when passing records? Would that work?
```java
class InternalRecordWrapper implements StructLike {
private final Function<Object, Object>[] transforms;
private StructLike wrapped = null;
@SuppressWarnings("unchecked")
InternalRecordWrapper(Types.StructType struct) {
this.transforms = struct.fields().stream()
.map(field -> converter(field.type()))
.toArray(length -> (Function<Object, Object>[]) Array.newInstance(Function.class, length));
}
private static Function<Object, Object> converter(Type type) {
switch (type.typeId()) {
case DATE:
return date -> DateTimeUtil.daysFromDate((LocalDate) date);
case TIME:
return time -> DateTimeUtil.microsFromTime((LocalTime) time);
case TIMESTAMP:
if (((Types.TimestampType) type).shouldAdjustToUTC()) {
return timestamp -> DateTimeUtil.microsFromTimestamptz((OffsetDateTime) timestamp);
} else {
return timestamp -> DateTimeUtil.microsFromTimestamp((LocalDateTime) timestamp);
}
case FIXED:
return bytes -> ByteBuffer.wrap((byte[]) bytes);
case STRUCT:
InternalRecordWrapper wrapper = new InternalRecordWrapper(type.asStructType());
return struct -> wrapper.wrap((StructLike) struct);
default:
}
return null;
}
public InternalRecordWrapper wrap(StructLike record) {
this.wrapped = record;
return this;
}
@Override
public int size() {
return wrapped.size();
}
@Override
public <T> T get(int pos, Class<T> javaClass) {
if (transforms[pos] != null) {
return javaClass.cast(transforms[pos].apply(wrapped.get(pos, Object.class)));
}
return wrapped.get(pos, javaClass);
}
@Override
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot update InternalRecordWrapper");
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-632781762
Also, to address your comment that the accessors approach is cleaner, we already have two copies of the accessors (in Spark and in API) and running evaluators on another object model would require another copy. Moving the conversion responsibility to a wrapper means we can use the same accessors in other places. And a wrapper also fits well with the need to adapt other record models to `StructLike`. We can build a `StructLike` wrapper for Avro records, for example.
This doesn't support lists and maps because those can't be used in expressions. If we start supporting expressions that operate on collections, we would need to build a visitor like you suggested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r422598298
##########
File path: data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
##########
@@ -449,6 +454,45 @@ private DataFile writeFile(String location, String filename, List<Record> record
}
}
+ @Test
+ public void testFilterWithComplexType() throws IOException {
+ if (format == FileFormat.PARQUET) {
Review comment:
This is fixed by #891.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r422598298
##########
File path: data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
##########
@@ -449,6 +454,45 @@ private DataFile writeFile(String location, String filename, List<Record> record
}
}
+ @Test
+ public void testFilterWithComplexType() throws IOException {
+ if (format == FileFormat.PARQUET) {
Review comment:
This works after #891 merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada closed pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada closed pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-620722327
Thanks, @chenjunjiedada, but I don't think this is the right solution to the problem. This adds quite a bit of logic to get and changes the class argument so that it is a type request -- so the record will convert a value to the requested type. But get is used in a tight loop and we don't want it to do that additional work to not only return a value, but detect what it is supposed to convert to and do that conversion. Also, this changes the contract of a public method and we don't want to make additional guarantees here.
Instead, the accessors that are build when binding the message to an expression should be used. This happens in Spark, where string values are converted from UTF8String to a CharSequence using a [StringAccessor](https://github.com/apache/incubator-iceberg/blob/master/spark/src/main/java/org/apache/iceberg/spark/source/PartitionKey.java#L260). This should be similar, where a DateAccessor, or TimestampAccessor is used to return the right internal representation from a record that stores LocalDate or OffsetDateTime.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] shardulm94 commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-634305850
@rdblue Ack, on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada edited a comment on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada edited a comment on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-633808288
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue merged pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-632956119
@rdblue, I have updated to use wrapper class, would you please take another look?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-633650626
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-632550875
@rdblue , The wrapper class tries to do something like the first commit I submit, IIUC, it should work but that would cause the concerns you raised before (impact perf in the tight loop and break the contact). Plus, the class doesn't handle the `LIST` and `MAP`, maybe we need a visitor to build transforms?
> I think what we need to do instead is to separate the concerns. Accessors should understand structure and rows should return the correct value types. That way we can eventually move to use the same accessors and we will just need rows that correctly translate for StructLike.
Either one of the concerns gets solved should work, right? Given that we will have more data models (PIG, HIVE, Flink and etc..), eventually we will need more accessors or wrapper classes. IMHO, the accessor way looks a bit cleaner, actually, we could make the functions and subclasses in `StructLike` accessor as public, so that others could inherit from that. Does this make sense to you?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-633770057
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r417465942
##########
File path: data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
##########
@@ -449,6 +454,45 @@ private DataFile writeFile(String location, String filename, List<Record> record
}
}
+ @Test
+ public void testFilterWithComplexType() throws IOException {
+ if (format == FileFormat.PARQUET) {
Review comment:
Tests should be disabled using `Assume`, so it doesn't look like they are passing and are skipped instead.
I think it makes sense to only disable the parts of the test that don't pass in Parquet -- I think just the timestamp without zone type. Can you separate that into a different case and add the assume to that one?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r417471819
##########
File path: api/src/main/java/org/apache/iceberg/Accessors.java
##########
@@ -187,6 +206,47 @@ public String toString() {
}
}
+ private static class DateAccessor extends PositionAccessor {
+ private DateAccessor(int pos, Type type) {
+ super(pos, type);
+ }
+
+ @Override
+ public Object get(StructLike record) {
+ LocalDate value = record.get(position(), LocalDate.class);
+ return (int) ChronoUnit.DAYS.between(EPOCH_DAY, value);
Review comment:
By moving these into `iceberg-data`, we can reuse the util class `DateTimeUtil`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-632778769
You're right that it's more similar to your original commit than to my suggestion to use accessors. The other issue with accessors made me think that conversion should not be tightly coupled there.
This is also a bit different from your original version: the `GenericRecord` class isn't modified. Instead, it is wrapped to produce the internal representations. The original version detected when it needed to convert based on the requested class, which is what I wanted to avoid. This converts values using a list of transforms determined when the wrapper is created. For example, if position 5 is a date, then it is always transformed when it is returned. This cuts down on the amount of work done fetching each value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r416534895
##########
File path: data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
##########
@@ -449,6 +454,45 @@ private DataFile writeFile(String location, String filename, List<Record> record
}
}
+ @Test
+ public void testFilterWithComplexType() throws IOException {
+ if (format == FileFormat.PARQUET) {
Review comment:
This is due to https://issues.apache.org/jira/browse/PARQUET-1851.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada edited a comment on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada edited a comment on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-621185042
Make sense to me, thanks @rdblue! Now I added accessors to `Accessors` in the api package (tried to build accessors for `GenericRecord` but it needs more basic changes).
The unit test `testDayPartitionedTimestampFilters` from testFilteredScan.java failed because transform.day doesn't produce date value but assume it is date type. I open https://github.com/apache/incubator-iceberg/issues/984 to track this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r417468584
##########
File path: api/src/main/java/org/apache/iceberg/Accessors.java
##########
@@ -187,6 +206,47 @@ public String toString() {
}
}
+ private static class DateAccessor extends PositionAccessor {
Review comment:
These accessors are for types used by Iceberg generics. I don't think that we want to make them part of the public API, and we need to be able to add them when using generics but not use them when binding to other rows. These should not be used with `PartitionData` instances, for example.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] waterlx commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
waterlx commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r420217792
##########
File path: api/src/main/java/org/apache/iceberg/Accessors.java
##########
@@ -187,6 +206,47 @@ public String toString() {
}
}
+ private static class DateAccessor extends PositionAccessor {
+ private DateAccessor(int pos, Type type) {
+ super(pos, type);
+ }
+
+ @Override
+ public Object get(StructLike record) {
+ LocalDate value = record.get(position(), LocalDate.class);
+ return (int) ChronoUnit.DAYS.between(EPOCH_DAY, value);
Review comment:
Does it make sense to have a separate PR to add those common functions into DateTimeUtil?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r417466810
##########
File path: api/src/test/java/org/apache/iceberg/transforms/TestResiduals.java
##########
@@ -191,26 +191,18 @@ public void testInTimestamp() {
Types.NestedField.optional(51, "dateint", Types.IntegerType.get())
);
- Long date20191201 = (Long) Literal.of("2019-12-01T00:00:00.00000")
- .to(Types.TimestampType.withoutZone()).value();
- Long date20191202 = (Long) Literal.of("2019-12-02T00:00:00.00000")
- .to(Types.TimestampType.withoutZone()).value();
-
PartitionSpec spec = PartitionSpec.builderFor(schema)
.day("ts")
.build();
- Transform day = spec.getFieldsBySourceId(50).get(0).transform();
- Integer tsDay = (Integer) day.apply(date20191201);
-
- Predicate pred = in("ts", date20191201, date20191202);
+ Predicate pred = in("ts", "2019-12-01T00:00:00.00000", "2019-12-01T00:00:00.00000");
Review comment:
Why were these changes necessary?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r417474026
##########
File path: api/src/main/java/org/apache/iceberg/Accessors.java
##########
@@ -187,6 +206,47 @@ public String toString() {
}
}
+ private static class DateAccessor extends PositionAccessor {
Review comment:
Looks like this is the cause of the test failures:
```
org.apache.iceberg.spark.source.TestFilteredScan > testDayPartitionedTimestampFilters[2] FAILED
java.lang.IllegalArgumentException: Wrong class, java.time.LocalDate, for object: 17521
at org.apache.iceberg.PartitionData.get(PartitionData.java:120)
at org.apache.iceberg.Accessors$DateAccessor.get(Accessors.java:216)
at org.apache.iceberg.Accessors$DateAccessor.get(Accessors.java:209)
at org.apache.iceberg.expressions.BoundReference.eval(BoundReference.java:39)
```
We need to substitute this accessor for the default accessor only when using Iceberg generics.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r419994243
##########
File path: api/src/test/java/org/apache/iceberg/transforms/TestResiduals.java
##########
@@ -191,26 +191,18 @@ public void testInTimestamp() {
Types.NestedField.optional(51, "dateint", Types.IntegerType.get())
);
- Long date20191201 = (Long) Literal.of("2019-12-01T00:00:00.00000")
- .to(Types.TimestampType.withoutZone()).value();
- Long date20191202 = (Long) Literal.of("2019-12-02T00:00:00.00000")
- .to(Types.TimestampType.withoutZone()).value();
-
PartitionSpec spec = PartitionSpec.builderFor(schema)
.day("ts")
.build();
- Transform day = spec.getFieldsBySourceId(50).get(0).transform();
- Integer tsDay = (Integer) day.apply(date20191201);
-
- Predicate pred = in("ts", date20191201, date20191202);
+ Predicate pred = in("ts", "2019-12-01T00:00:00.00000", "2019-12-01T00:00:00.00000");
Review comment:
The unit tests failed with the following exception, the same reason that tranform.day doesn't produce the date value. These changes are not necessary just use to pass unit tests. I will revert the changes.
```
Cannot cast java.lang.Integer to java.time.LocalDate
java.lang.ClassCastException: Cannot cast java.lang.Integer to java.time.LocalDate
at java.lang.Class.cast(Class.java:3369)
at org.apache.iceberg.TestHelpers$Row.get(TestHelpers.java:121)
at org.apache.iceberg.Accessors$DateAccessor.get(Accessors.java:216)
at org.apache.iceberg.Accessors$DateAccessor.get(Accessors.java:209)
at org.apache.iceberg.expressions.BoundReference.eval(BoundReference.java:39)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.in(ResidualEvaluator.java:201)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.in(ResidualEvaluator.java:120)
at org.apache.iceberg.expressions.ExpressionVisitors$BoundExpressionVisitor.predicate(ExpressionVisitors.java:152)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.predicate(ResidualEvaluator.java:258)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.predicate(ResidualEvaluator.java:281)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.predicate(ResidualEvaluator.java:120)
at org.apache.iceberg.expressions.ExpressionVisitors.visit(ExpressionVisitors.java:283)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.eval(ResidualEvaluator.java:125)
at org.apache.iceberg.expressions.ResidualEvaluator$ResidualVisitor.access$100(ResidualEvaluator.java:120)
at org.apache.iceberg.expressions.ResidualEvaluator.residualFor(ResidualEvaluator.java:117)
at org.apache.iceberg.transforms.TestResiduals.testInTimestamp(TestResiduals.java:211)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r422598070
##########
File path: api/src/main/java/org/apache/iceberg/Accessors.java
##########
@@ -187,6 +206,47 @@ public String toString() {
}
}
+ private static class DateAccessor extends PositionAccessor {
Review comment:
@rdblue , I just use another solution for this. Take `accessors` from passing parameter instead of parsing it from schema when binding expression with struct type. With this decoupling we could customize the `accessors` for different `StructLike`. Does that make sense to you?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#issuecomment-621185042
@rdblue, I added accessors to `Accessors` in the api package (tried to build accessors for `GenericRecord` but it needs more basic changes).
The unit test `testDayPartitionedTimestampFilters` from testFilteredScan.java failed because transform.day doesn't produce date value but assume it is date type. I open https://github.com/apache/incubator-iceberg/issues/984 to track this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r430016878
##########
File path: data/src/main/java/org/apache/iceberg/data/TableScanIterable.java
##########
@@ -161,7 +161,10 @@ public boolean hasNext() {
if (task.residual() != null && task.residual() != Expressions.alwaysTrue()) {
Evaluator filter = new Evaluator(projection.asStruct(), task.residual(), caseSensitive);
- this.currentIterator = Iterables.filter(reader, filter::eval).iterator();
+ this.currentIterator = Iterables.filter(reader, record -> {
+ InternalRecordWrapper wrapperRecord = new InternalRecordWrapper(record.struct()).wrap(record);
Review comment:
The wrapper can be reused so it should be moved outside the predicate. The predicate just needs to be this:
```java
record -> filter.eval(wrapper.wrap(record))
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull request #983: Convert date and timestamp values in generics
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #983:
URL: https://github.com/apache/incubator-iceberg/pull/983#discussion_r420016471
##########
File path: api/src/main/java/org/apache/iceberg/Accessors.java
##########
@@ -187,6 +206,47 @@ public String toString() {
}
}
+ private static class DateAccessor extends PositionAccessor {
Review comment:
I agree that these accessors should be specific for Iceberg generics and basically there are three options to do that:
1. Identify Iceberg generic class in `Accessors` in iceberg-api, but that would mix in iceberg-data into iceberg-api package.
2. Copy all related `Accessors` into iceberg-data package, but `Evaluator` constructs accessors via `Schema#accessForField` which comes from iceberg-api package, so that we can not pass iceberg generic accessors to them. Or we need to update Evaluator but it is still in iceberg-api package.
3. Fix the issue: https://github.com/apache/incubator-iceberg/issues/984, so that DateAccessor works for PartitionData as well.
IMO, option 3 is feasible, what do you think? Or you may have other options.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org