You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zeruibao (via GitHub)" <gi...@apache.org> on 2023/06/08 18:54:36 UTC
[GitHub] [spark] zeruibao opened a new pull request, #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
zeruibao opened a new pull request, #41521:
URL: https://github.com/apache/spark/pull/41521
### What changes were proposed in this pull request?
Fix conversion of Avro logical timestamp type to Long
### Why are the changes needed?
The fix in https://github.com/apache/spark/pull/41052 cause regression when trying to convert logical timestamp type to Long type and it's not covered by existing test suite. Fix it and add a test for it.
### Does this PR introduce _any_ user-facing change?
NO
### How was this patch tested?
New unit test
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223944833
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
I always read them as long with `updater.setLong(ordinal, value.asInstanceOf[Long])` In the past, we don't care about LogicalType at all.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1224518498
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros |
+ _: TimeMicros | _: TimeMillis => (updater, ordinal, value) =>
Review Comment:
Why don't we handle it here. Logical type TimeMicros & TimeMillis can also convert to long type.
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros |
+ _: TimeMicros | _: TimeMillis => (updater, ordinal, value) =>
Review Comment:
Why don't we handle it here? Logical type TimeMicros & TimeMillis can also convert to long 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223884078
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
Sorry, I didn't get how you can handle in the same way milliseconds and microseconds.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1224518498
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros |
+ _: TimeMicros | _: TimeMillis => (updater, ordinal, value) =>
Review Comment:
Why don't we handle it here? Look at the test suite I mention above. Data type `Long` Logical type `TimeMicros` & `TimeMillis` can also convert to long 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1224523551
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
I just follow the behavior of this test suite in runtime repo
```
Seq(
"time-millis",
"time-micros",
"timestamp-micros",
"local-timestamp-millis",
"local-timestamp-micros"
).foreach { timeLogicalType =>
val timestampSchema = timestampSchemaTemplate.format(timeLogicalType)
if (DatabricksEdgeConfigs.edgeModeEnabled) {
test(s"$timeLogicalType type write support of Avro Hive serde") {
Seq(true, false).foreach { useNativeAvro =>
withSQLConf(DatabricksSQLConf.CONVERT_METASTORE_AVRO, useNativeAvro) {
val tableName = "tab1"
withTable(tableName) {
// Creates the (non-)partitioned Avro table
versionSpark.sql(
s"""
|CREATE TABLE $tableName
|ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.avro.AvroSerDe'
|STORED AS
| INPUTFORMAT 'org.apache.hadoop.hive.ql.io.avro.AvroContainerInputFormat'
| OUTPUTFORMAT 'org.apache.hadoop.hive.ql.io.avro.AvroContainerOutputFormat'
|TBLPROPERTIES ('avro.schema.literal' = '$timestampSchema')
""".stripMargin
)
val insertStmt = s"INSERT OVERWRITE TABLE $tableName SELECT 1000"
val result = versionSpark.sql(s"SELECT 1000").collect()
versionSpark.sql(insertStmt)
assert(versionSpark.table(tableName).collect() === result)
}
}
}
}
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1227337246
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,13 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros => (updater, ordinal, value) =>
+ updater.setLong(ordinal, value.asInstanceOf[Long])
+ case other => throw new IncompatibleSchemaException(errorPrefix +
Review Comment:
Not yet. Let me add 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
URL: https://github.com/apache/spark/pull/41521
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1224542375
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros |
+ _: TimeMicros | _: TimeMillis => (updater, ordinal, value) =>
Review Comment:
Yeah, you are right. It's already handled. Thank 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1227336131
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -946,6 +946,35 @@ abstract class AvroSuite
}
}
+ Seq(
+ "time-millis",
Review Comment:
@zeruibao can you show me where is the `time-millis` and `time-micros` handled in the deserializer?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223853665
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -158,7 +158,7 @@ private[sql] class AvroDeserializer(
}
case LONG =>
(logicalDataType, catalystType) match {
- case (LongType, LongType) => (updater, ordinal, value) =>
+ case (_, LongType) => (updater, ordinal, value) =>
Review Comment:
Got it. Let me smaller the scope. Now it's only allow the logical type
```
"time-millis",
"time-micros",
"timestamp-micros",
"timestamp-millis",
"local-timestamp-millis",
"local-timestamp-micros"
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223944833
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
I always read them as long with `updater.setLong(ordinal, value.asInstanceOf[Long])` In the past, we don't care about LogicalType at all. code look like this
```
case (LONG, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
```
As long as the encoded type is Long, we always handle it in the same way.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223953659
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros |
+ _: TimeMicros | _: TimeMillis => (updater, ordinal, value) =>
Review Comment:
Why do we handle TimeMicros & TimeMillis here?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223884078
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
Sorry, I didn't get how you can handle milliseconds and microseconds in the same way.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1227336291
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,13 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros => (updater, ordinal, value) =>
+ updater.setLong(ordinal, value.asInstanceOf[Long])
+ case other => throw new IncompatibleSchemaException(errorPrefix +
Review Comment:
Is there a test case for this code path?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223900560
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
good point. @zeruibao what was the behavior before?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1224518498
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
+ _: LocalTimestampMillis | _: LocalTimestampMicros |
+ _: TimeMicros | _: TimeMillis => (updater, ordinal, value) =>
Review Comment:
Why don't we handle it here? Look at the test suite I mention above. Data type `Long` Logical type `TimeMicros` & `TimeMillis` can also convert to long 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #41521: [SPARK-43380][SQL][FOLLOWUP] Fix conversion of Avro logical timestamp type to Long
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41521:
URL: https://github.com/apache/spark/pull/41521#issuecomment-1589658756
Thank you, @zeruibao , @cloud-fan , @MaxGekk , @gengliangwang .
Merged to master for Apache Spark 3.5.0.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on PR #41521:
URL: https://github.com/apache/spark/pull/41521#issuecomment-1583330021
Yeah, I think https://github.com/apache/spark/pull/41052 is only merged to master branch. @dongjoon-hyun
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1227352642
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -946,6 +946,35 @@ abstract class AvroSuite
}
}
+ Seq(
+ "time-millis",
Review Comment:
When the logical type is `time-millis` and `time-micros` , `SchemaConverters.toSqlType(avroType).dataType` is also longType. So, it's handled.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223952202
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
Could it be a long standing bug?
I am seeing:
https://github.com/apache/spark/pull/41052/files#diff-2eaa6c4b7371cae9800cff1cee843a6245ab91206595cf0e403c077ab2ba80daL138
```
// For backward compatibility, if the Avro type is Long and it is not logical type
// (the `null` case), the value is processed as timestamp type with millisecond precision.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1223844129
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -158,7 +158,7 @@ private[sql] class AvroDeserializer(
}
case LONG =>
(logicalDataType, catalystType) match {
- case (LongType, LongType) => (updater, ordinal, value) =>
+ case (_, LongType) => (updater, ordinal, value) =>
Review Comment:
so any physical avro long type can be read as spark long type? does it mean we can read a daytime interval in avro as long 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zeruibao commented on a diff in pull request #41521: [SPARK-43380][SQL] Fix conversion of Avro logical timestamp type to Long
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on code in PR #41521:
URL: https://github.com/apache/spark/pull/41521#discussion_r1224551712
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -160,6 +160,14 @@ private[sql] class AvroDeserializer(
(logicalDataType, catalystType) match {
case (LongType, LongType) => (updater, ordinal, value) =>
updater.setLong(ordinal, value.asInstanceOf[Long])
+ case (_, LongType) => avroType.getLogicalType match {
+ case _: TimestampMicros | _: TimestampMillis |
Review Comment:
shall I still follow the original behavior?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org