You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/08/07 13:22:29 UTC
[GitHub] [iceberg] nastra opened a new pull request, #8250: Spark 3.4: Fix issue when partitioning by UUID
nastra opened a new pull request, #8250:
URL: https://github.com/apache/iceberg/pull/8250
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
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] [iceberg] nastra commented on a diff in pull request #8250: Spark 3.4: Fix issue when partitioning by UUID
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1286737815
##########
data/src/test/java/org/apache/iceberg/RecordWrapperTest.java:
##########
@@ -44,8 +44,8 @@ public abstract class RecordWrapperTest {
optional(113, "bytes", Types.BinaryType.get()),
required(114, "dec_9_0", Types.DecimalType.of(9, 0)),
required(115, "dec_11_2", Types.DecimalType.of(11, 2)),
- required(116, "dec_38_10", Types.DecimalType.of(38, 10)) // maximum precision
- );
+ required(116, "dec_38_10", Types.DecimalType.of(38, 10)), // maximum precision
+ optional(117, "uuid", Types.UUIDType.get()));
Review Comment:
this is being used by Spark tests across all versions, so we need to apply the fix for all Spark versions in a single PR unfortunately
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark: Fix issue when partitioning by UUID [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1361595378
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -40,9 +43,12 @@ class InternalRowWrapper implements StructLike {
private InternalRow row = null;
@SuppressWarnings("unchecked")
- InternalRowWrapper(StructType rowType) {
+ InternalRowWrapper(StructType rowType, Types.StructType icebergStruct) {
Review Comment:
I've added 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark 3.4: Fix issue when partitioning by UUID [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1360824697
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -71,8 +77,12 @@ public <T> void set(int pos, T value) {
row.update(pos, value);
}
- private static BiFunction<InternalRow, Integer, ?> getter(DataType type) {
+ private static BiFunction<InternalRow, Integer, ?> getter(Type icebergType, DataType type) {
Review Comment:
I'd probably just keep it `type` to cause less changes
--
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: issues-unsubscribe@iceberg.apache.org
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] [iceberg] nastra commented on pull request #8250: Spark 3.4: Fix issue when partitioning by UUID
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#issuecomment-1673518654
@aokolnychyi could you take a look at this please?
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark 3.4: Fix issue when partitioning by UUID [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1359495723
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -71,8 +77,12 @@ public <T> void set(int pos, T value) {
row.update(pos, value);
}
- private static BiFunction<InternalRow, Integer, ?> getter(DataType type) {
+ private static BiFunction<InternalRow, Integer, ?> getter(Type icebergType, DataType type) {
if (type instanceof StringType) {
+ if (Type.TypeID.UUID == icebergType.typeId()) {
Review Comment:
can we add a comment here explaining that a StringType may be a UUID-type hence return UUID also should we explicity check the else case saying if it's not StringType either throw UnReachable or Unsupported ops or null accordingly ?
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -40,9 +43,12 @@ class InternalRowWrapper implements StructLike {
private InternalRow row = null;
@SuppressWarnings("unchecked")
- InternalRowWrapper(StructType rowType) {
+ InternalRowWrapper(StructType rowType, Types.StructType icebergStruct) {
Review Comment:
should we have a Pre-condition check that icebergStruct.fields().length is equal to types.lenght ?
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark: Fix issue when partitioning by UUID [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1603639825
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -40,9 +44,17 @@ class InternalRowWrapper implements StructLike {
private InternalRow row = null;
@SuppressWarnings("unchecked")
- InternalRowWrapper(StructType rowType) {
+ InternalRowWrapper(StructType rowType, Types.StructType icebergStruct) {
Review Comment:
makes sense, I've updated that
--
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: issues-unsubscribe@iceberg.apache.org
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] [iceberg] hililiwei commented on a diff in pull request #8250: Spark 3.4: Fix issue when partitioning by UUID
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1287852686
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -71,8 +77,12 @@ public <T> void set(int pos, T value) {
row.update(pos, value);
}
- private static BiFunction<InternalRow, Integer, ?> getter(DataType type) {
+ private static BiFunction<InternalRow, Integer, ?> getter(Type icebergType, DataType type) {
Review Comment:
nit: Do we rename the `type` to make it clear that it is `sql type` of spark?
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark: Fix issue when partitioning by UUID [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #8250:
URL: https://github.com/apache/iceberg/pull/8250
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark: Fix issue when partitioning by UUID [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#discussion_r1603587516
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/InternalRowWrapper.java:
##########
@@ -40,9 +44,17 @@ class InternalRowWrapper implements StructLike {
private InternalRow row = null;
@SuppressWarnings("unchecked")
- InternalRowWrapper(StructType rowType) {
+ InternalRowWrapper(StructType rowType, Types.StructType icebergStruct) {
Review Comment:
Nit: I think I'd call it `icebergSchema` instead of `icebergStruct`, it's not an actual Iceberg Schema type, but I think that's a more helpful name to indicate that it's a representation of the iceberg schema.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Spark: Fix issue when partitioning by UUID [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#issuecomment-2115675915
thanks for the reviews @hililiwei, @singhpk234, @amogh-jahagirdar
--
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: issues-unsubscribe@iceberg.apache.org
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] [iceberg] manuzhang commented on pull request #8250: Spark 3.4: Fix issue when partitioning by UUID
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #8250:
URL: https://github.com/apache/iceberg/pull/8250#issuecomment-1668771951
Which issue is this fixing?
--
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: issues-unsubscribe@iceberg.apache.org
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