You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zzzzming95 (via GitHub)" <gi...@apache.org> on 2024/03/09 10:32:05 UTC
[PR] Support convert orc timestamptz [iceberg]
zzzzming95 opened a new pull request, #9905:
URL: https://github.com/apache/iceberg/pull/9905
### What changes were proposed in this pull request?
A user was attempting to convert an ORC backed external table in hive to a Iceberg table using the migrate command but was immediately met with a "Can not promote TIMESTAMP to TIMESTAMP" error. This occurs because our Spark -> Iceberg conversion code always converts to a Timestamp.withZone.
it relate issue : https://github.com/apache/iceberg/issues/9784
```
spark.sql("CREATE EXTERNAL TABLE mytable (foo timestamp) STORED AS orc LOCATION '/Users/russellspitzer/Temp/foo'")
spark.sql("INSERT INTO mytable VALUES (now())")
spark.sql("CALL spark_catalog.system.migrate('mytable')")
spark.sql("SELECT * FROM mytable")
```
```
java.lang.IllegalArgumentException: Can not promote TIMESTAMP type to TIMESTAMP
at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:441)
at org.apache.iceberg.orc.ORCSchemaUtil.buildOrcProjection(ORCSchemaUtil.java:301)
at org.apache.iceberg.orc.ORCSchemaUtil.buildOrcProjection(ORCSchemaUtil.java:275)
at org.apache.iceberg.orc.ORCSchemaUtil.buildOrcProjection(ORCSchemaUtil.java:258)
```
add a config , `iceberg.orc.convert.timestamptz` , when set is as true , it will auto convert orc `TIMESTAMP` as orc `TIMESTAMP_INSTANT` , so we can fix this issue.
### Why are the changes needed?
fix issue.
### Does this PR introduce any user-facing change?
No
### How was this patch tested?
add a UT
--
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] Support convert orc timestamptz [iceberg]
Posted by "zzzzming95 (via GitHub)" <gi...@apache.org>.
zzzzming95 commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1677119029
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -296,24 +304,36 @@ private static TypeDescription buildOrcProjection(
list.elementId(),
list.elementType(),
isRequired && list.isElementRequired(),
- mapping);
+ mapping,
+ config);
orcType = TypeDescription.createList(elementType);
break;
case MAP:
Types.MapType map = (Types.MapType) type;
TypeDescription keyType =
- buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping);
+ buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping, config);
TypeDescription valueType =
buildOrcProjection(
- map.valueId(), map.valueType(), isRequired && map.isValueRequired(), mapping);
+ map.valueId(),
+ map.valueType(),
+ isRequired && map.isValueRequired(),
+ mapping,
+ config);
orcType = TypeDescription.createMap(keyType, valueType);
break;
default:
if (mapping.containsKey(fieldId)) {
TypeDescription originalType = mapping.get(fieldId).type();
Optional<TypeDescription> promotedType = getPromotedType(type, originalType);
- if (promotedType.isPresent()) {
+ boolean convertTimestampTZ =
+ config.getBoolean(ConfigProperties.ORC_CONVERT_TIMESTAMPTZ, false);
+
+ if (convertTimestampTZ
+ && type.typeId() == Type.TypeID.TIMESTAMP
Review Comment:
Some of the processing logic here involves more than just the timestamp type, so maybe an if is more appropriate
--
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] Support convert orc timestamptz [iceberg]
Posted by "zzzzming95 (via GitHub)" <gi...@apache.org>.
zzzzming95 commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2285382336
> > > @zzzzming95: What happens if someone writes to this ORC table some new rows after migration?
> > > @pvary
> >
> >
> > Sorry I've had a recent job change, so I don't have an environment to test this case right now. But I understand that there shouldn't be a problem, because 'shouldAdjustToUTC()' will find the correct timestamp type.
>
> Could we add a test with actual data files with the new, old, mixed format?
Yes, I also think such a UT is needed, I will try to add it later.
--
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] Support convert orc timestamptz [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2427964374
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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] Support convert orc timestamptz [iceberg]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2216473669
@snazy @nastra @rdblue
I confirmed that this issue is happening on my env (Spark 3.4, Iceberg 1.3.1) as well and this is blocking my team from migrating our Hive tables to Iceberg.
Could you please take a look at this PR? If there is anything I can do, please let me know (cc @zzzzming95 )
--
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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1714688639
##########
orc/src/test/java/org/apache/iceberg/orc/TestExpressionToSearchArgument.java:
##########
@@ -332,6 +334,7 @@ public void testEvolvedSchema() {
@Test
public void testOriginalSchemaNameMapping() {
+ Configuration config = new Configuration();
Review Comment:
Is this used anywhere?
--
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] Support convert orc timestamptz [iceberg]
Posted by "zzzzming95 (via GitHub)" <gi...@apache.org>.
zzzzming95 commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1677119220
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -23,7 +23,9 @@
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
+import org.apache.hadoop.conf.Configuration;
Review Comment:
fix as boolean~
--
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] Support convert orc timestamptz [iceberg]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2219743020
@zzzzming95
Do you have time and effort for this issue? (I would appreciate if you could)
If not, I might create a new PR based from 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.
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] Support convert orc timestamptz [iceberg]
Posted by "zzzzming95 (via GitHub)" <gi...@apache.org>.
zzzzming95 commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2219754164
@tanvn yeah, i will continue this issue this week~
--
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] Support convert orc timestamptz [iceberg]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2232200121
@nastra @snazy
I think your comments have been addressed by @zzzzming95
Could you take another look 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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2252026580
@deniskuzZ: How this is solved in Hive?
--
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] Support convert orc timestamptz [iceberg]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2281371223
@zzzzming95
Could you take another look at the comments when you have time? 🙇
--
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] Support convert orc timestamptz [iceberg]
Posted by "zzzzming95 (via GitHub)" <gi...@apache.org>.
zzzzming95 commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2236449363
> On #9784 you mentioned:
>
> > I think this is because hive and spark treat `timestamp` data type as timestamp with time zone and the orc file format is also stored as orc `timestamp` type. But in fact the hive `timestamp` data type should be stored as `timestamp_instant` in the orc file.
>
> This sounds like Hive and Spark treat the `timestamp` type in a "less flexible way". IIRC Spark 3.4 introduced `timestamp_ntz`, whereas Hive uses no timezone at all.
>
> The `iceberg.orc.convert.timestamptz` option introduced with this PR seems to be a global setting, so it affects all tables. I wonder whether this should rather be an ORC type property.
Although the `timestamp_ntz` type is introduced in Spark 3.4+, this type is actually stored as the `array<bigint>`data type in orc file.
The purpose of `iceberg.orc.convert.timestamptz` is to provide hive spark with a compatible way to access the orc timestamp data type.
I am not quite sure what `I wonder whether this should rather be an ORC type property` means. If it means adding a new type to orc, such as `timestamptz`, this still cannot solve the incompatible access of the historical orc timestamp type.
--
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] Support convert orc timestamptz [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1670374109
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -296,24 +304,36 @@ private static TypeDescription buildOrcProjection(
list.elementId(),
list.elementType(),
isRequired && list.isElementRequired(),
- mapping);
+ mapping,
+ config);
orcType = TypeDescription.createList(elementType);
break;
case MAP:
Types.MapType map = (Types.MapType) type;
TypeDescription keyType =
- buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping);
+ buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping, config);
TypeDescription valueType =
buildOrcProjection(
- map.valueId(), map.valueType(), isRequired && map.isValueRequired(), mapping);
+ map.valueId(),
+ map.valueType(),
+ isRequired && map.isValueRequired(),
+ mapping,
+ config);
orcType = TypeDescription.createMap(keyType, valueType);
break;
default:
if (mapping.containsKey(fieldId)) {
TypeDescription originalType = mapping.get(fieldId).type();
Optional<TypeDescription> promotedType = getPromotedType(type, originalType);
- if (promotedType.isPresent()) {
+ boolean convertTimestampTZ =
+ config.getBoolean(ConfigProperties.ORC_CONVERT_TIMESTAMPTZ, false);
+
+ if (convertTimestampTZ
+ && type.typeId() == Type.TypeID.TIMESTAMP
Review Comment:
Guess this deserves a new `case TIMESTAMP`, not an `if` here.
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -23,7 +23,9 @@
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
+import org.apache.hadoop.conf.Configuration;
Review Comment:
Wonder whether the "whole `Configuration` spiel" is necessary here. Hadoop dependencies are already `compileOnly` to eventually get rid of those entirely. Wouldn't a simple boolean flag do the same thing in this class?
--
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] Support convert orc timestamptz [iceberg]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2428460185
@zzzzming95
Sorry for bothering you again, may I ask if you could dedicate some effort to 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.
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] Support convert orc timestamptz [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2508748226
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2255169998
> > @deniskuzZ: How this is solved in Hive?
>
> i think it's done in `Hive: Support timestamp with local zone in Hive3 (#1897)`
#1897 does not have any ORC specific part. I still miss some part of the puzzle...
@zzzzming95: What happens if someone writes to this ORC table some new rows after migration?
--
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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1694724039
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -296,24 +304,36 @@ private static TypeDescription buildOrcProjection(
list.elementId(),
list.elementType(),
isRequired && list.isElementRequired(),
- mapping);
+ mapping,
+ config);
orcType = TypeDescription.createList(elementType);
break;
case MAP:
Types.MapType map = (Types.MapType) type;
TypeDescription keyType =
- buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping);
+ buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping, config);
TypeDescription valueType =
buildOrcProjection(
- map.valueId(), map.valueType(), isRequired && map.isValueRequired(), mapping);
+ map.valueId(),
+ map.valueType(),
+ isRequired && map.isValueRequired(),
+ mapping,
+ config);
orcType = TypeDescription.createMap(keyType, valueType);
break;
default:
if (mapping.containsKey(fieldId)) {
TypeDescription originalType = mapping.get(fieldId).type();
Optional<TypeDescription> promotedType = getPromotedType(type, originalType);
- if (promotedType.isPresent()) {
+ boolean convertTimestampTZ =
+ config.getBoolean(ConfigProperties.ORC_CONVERT_TIMESTAMPTZ, false);
+
+ if (convertTimestampTZ
+ && type.typeId() == Type.TypeID.TIMESTAMP
Review Comment:
Why is this logic is not part of the `getPromotedType` method?
--
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] Support convert orc timestamptz [iceberg]
Posted by "zzzzming95 (via GitHub)" <gi...@apache.org>.
zzzzming95 commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2285146229
> @zzzzming95: What happens if someone writes to this ORC table some new rows after migration?
@pvary
Sorry I've had a recent job change, so I don't have an environment to test this case right now. But I understand that there shouldn't be a problem, because 'shouldAdjustToUTC()' will find the correct timestamp type.
--
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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1714689121
##########
orc/src/test/java/org/apache/iceberg/orc/TestExpressionToSearchArgument.java:
##########
@@ -358,6 +363,7 @@ public void testOriginalSchemaNameMapping() {
@Test
public void testModifiedSimpleSchemaNameMapping() {
+ Configuration config = new Configuration();
Review Comment:
Is this used anywhere?
--
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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1714689354
##########
orc/src/test/java/org/apache/iceberg/orc/TestExpressionToSearchArgument.java:
##########
@@ -396,6 +402,7 @@ public void testModifiedSimpleSchemaNameMapping() {
@Test
public void testModifiedComplexSchemaNameMapping() {
+ Configuration config = new Configuration();
Review Comment:
Same as above
--
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] Support convert orc timestamptz [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1669926992
##########
.palantir/revapi.yml:
##########
@@ -1018,6 +1018,13 @@ acceptedBreaks:
old: "method void org.apache.iceberg.PositionDeletesTable.PositionDeletesBatchScan::<init>(org.apache.iceberg.Table,\
\ org.apache.iceberg.Schema, org.apache.iceberg.TableScanContext)"
justification: "Removing deprecated code"
+ org.apache.iceberg:iceberg-orc:
Review Comment:
we can't break existing APIs but you could add an overloaded version of that method that takes an additional parameter. That way it won't break the existing API. See also https://iceberg.apache.org/contribute/#adding-new-functionality-without-breaking-apis
--
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] Support convert orc timestamptz [iceberg]
Posted by "pravin1406 (via GitHub)" <gi...@apache.org>.
pravin1406 commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2576237229
@zzzzming95 I was facing this original issue and went about solving in the way you did, but it didn't work out for me,it was giving incorrect timestamp (future +5:30 (IST)).
I see my solution is almost exactly as yours. Can you confirm if you tested your PR ? Did it work ?
--
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] Support convert orc timestamptz [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2285375168
> > @zzzzming95: What happens if someone writes to this ORC table some new rows after migration?
> > @pvary
>
> Sorry I've had a recent job change, so I don't have an environment to test this case right now. But I understand that there shouldn't be a problem, because 'shouldAdjustToUTC()' will find the correct timestamp type.
Could we add a test with actual data files with the new, old, mixed format?
--
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] Support convert orc timestamptz [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2492612301
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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] Support convert orc timestamptz [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9905: Support convert orc timestamptz
URL: https://github.com/apache/iceberg/pull/9905
--
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] Support convert orc timestamptz [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#discussion_r1680492194
##########
orc/src/test/java/org/apache/iceberg/orc/TestBuildOrcProjection.java:
##########
@@ -23,7 +23,9 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import org.apache.hadoop.conf.Configuration;
Review Comment:
Please clean those up
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -296,24 +304,36 @@ private static TypeDescription buildOrcProjection(
list.elementId(),
list.elementType(),
isRequired && list.isElementRequired(),
- mapping);
+ mapping,
+ config);
orcType = TypeDescription.createList(elementType);
break;
case MAP:
Types.MapType map = (Types.MapType) type;
TypeDescription keyType =
- buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping);
+ buildOrcProjection(map.keyId(), map.keyType(), isRequired, mapping, config);
TypeDescription valueType =
buildOrcProjection(
- map.valueId(), map.valueType(), isRequired && map.isValueRequired(), mapping);
+ map.valueId(),
+ map.valueType(),
+ isRequired && map.isValueRequired(),
+ mapping,
+ config);
orcType = TypeDescription.createMap(keyType, valueType);
break;
default:
if (mapping.containsKey(fieldId)) {
TypeDescription originalType = mapping.get(fieldId).type();
Optional<TypeDescription> promotedType = getPromotedType(type, originalType);
- if (promotedType.isPresent()) {
+ boolean convertTimestampTZ =
+ config.getBoolean(ConfigProperties.ORC_CONVERT_TIMESTAMPTZ, false);
+
+ if (convertTimestampTZ
+ && type.typeId() == Type.TypeID.TIMESTAMP
Review Comment:
Fair enough
##########
orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java:
##########
@@ -261,12 +261,30 @@ public static Schema convert(TypeDescription orcSchema) {
*/
public static TypeDescription buildOrcProjection(
Schema schema, TypeDescription originalOrcSchema) {
+ return buildOrcProjection(schema, originalOrcSchema, false);
+ }
+
+ /**
+ * Overload function `buildOrcProjection`
+ *
+ * @param schema an Iceberg schema
+ * @param originalOrcSchema an existing ORC file schema
+ * @param convertTimestampTZ should convert timestamptz as timestamp
+ * @return the resulting ORC schema
+ */
+ public static TypeDescription buildOrcProjection(
+ Schema schema, TypeDescription originalOrcSchema, Boolean convertTimestampTZ) {
final Map<Integer, OrcField> icebergToOrc = icebergToOrcMapping("root", originalOrcSchema);
- return buildOrcProjection(Integer.MIN_VALUE, schema.asStruct(), true, icebergToOrc);
+ return buildOrcProjection(
+ Integer.MIN_VALUE, schema.asStruct(), true, icebergToOrc, convertTimestampTZ);
}
private static TypeDescription buildOrcProjection(
- Integer fieldId, Type type, boolean isRequired, Map<Integer, OrcField> mapping) {
+ Integer fieldId,
+ Type type,
+ boolean isRequired,
+ Map<Integer, OrcField> mapping,
+ Boolean convertTimestampTZ) {
Review Comment:
This shouldn't be a boxed type
--
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] Support convert orc timestamptz [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2216851802
can you please rebase the PR and resolve conflicts? I'll try to take a closer look this week
--
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] Support convert orc timestamptz [iceberg]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2252907580
@raunaqmorarka please take a 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.
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] Support convert orc timestamptz [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #9905:
URL: https://github.com/apache/iceberg/pull/9905#issuecomment-2252944740
> @deniskuzZ: How this is solved in Hive?
i think it's done in `Hive: Support timestamp with local zone in Hive3 (#1897)`
--
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