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/08/10 02:58:19 UTC
[GitHub] [spark] zeruibao opened a new pull request, #42424: [SPARK-43380][SQL] Fix Avro data type conversion issues with causing performance regression
zeruibao opened a new pull request, #42424:
URL: https://github.com/apache/spark/pull/42424
### What changes were proposed in this pull request?
My last PR https://github.com/apache/spark/pull/41052 causes AVRO read performance regression since I change the code structure. I turn one match case into a nested match case. So I fix the Avro data type conversion issues in anther way to avoid this regression.
Original Change:
We introduce the SQLConf `spark.sql.legacy.avro.allowReadingWithIncompatibleSchema` to prevent reading interval types as date or timestamp types to avoid getting corrupt dates as well as reading decimal types with incorrect precision.
### Why are the changes needed?
We found the following issues with open source Avro:
- Interval types can be read as date or timestamp types that would lead to wildly different results
For example, `Duration.ofDays(1).plusSeconds(1)` will be read as `1972-09-27`, which is weird.
- Decimal types can be read with lower precision, that leads to data being read as `null` instead of suggesting that a wider decimal format should be provided
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Old 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 pull request #42424: [SPARK-43380][SQL] Fix Avro data type conversion issues without causing performance regression
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao commented on PR #42424:
URL: https://github.com/apache/spark/pull/42424#issuecomment-1679360581
Close it since it is duplicated.
--
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 pull request #42424: [SPARK-43380][SQL] Fix Avro data type conversion issues without causing performance regression
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #42424:
URL: https://github.com/apache/spark/pull/42424#issuecomment-1674372093
If the previous commit is problematic, please open a PR to revert it first. Then we can re-review your new fix. This PR is really hard to review as it mixes code structure changes and code logic 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: 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 #42424: [SPARK-43380][SQL] Fix Avro data type conversion issues without causing performance regression
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42424:
URL: https://github.com/apache/spark/pull/42424#discussion_r1289540318
##########
connector/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala:
##########
@@ -118,268 +118,225 @@ private[sql] class AvroDeserializer(
val incompatibleMsg = errorPrefix +
s"schema is incompatible (avroType = $avroType, sqlType = ${catalystType.sql})"
+ val realDataType = SchemaConverters.toSqlType(avroType).dataType
val confKey = SQLConf.LEGACY_AVRO_ALLOW_INCOMPATIBLE_SCHEMA
val preventReadingIncorrectType = !SQLConf.get.getConf(confKey)
- val logicalDataType = SchemaConverters.toSqlType(avroType).dataType
- avroType.getType match {
- case NULL =>
- (logicalDataType, catalystType) match {
- case (_, NullType) => (updater, ordinal, _) =>
- updater.setNullAt(ordinal)
- case _ => throw new IncompatibleSchemaException(incompatibleMsg)
- }
+ (avroType.getType, catalystType) match {
+ case (NULL, NullType) => (updater, ordinal, _) =>
+ updater.setNullAt(ordinal)
+
// TODO: we can avoid boxing if future version of avro provide primitive accessors.
- case BOOLEAN =>
- (logicalDataType, catalystType) match {
- case (_, BooleanType) => (updater, ordinal, value) =>
- updater.setBoolean(ordinal, value.asInstanceOf[Boolean])
- case _ => throw new IncompatibleSchemaException(incompatibleMsg)
- }
+ case (BOOLEAN, BooleanType) => (updater, ordinal, value) =>
+ updater.setBoolean(ordinal, value.asInstanceOf[Boolean])
+
+ case (INT, IntegerType) => (updater, ordinal, value) =>
+ updater.setInt(ordinal, value.asInstanceOf[Int])
+
+ case (LONG, dt: TimestampType)
+ if preventReadingIncorrectType && realDataType.isInstanceOf[DayTimeIntervalType] =>
+ throw QueryCompilationErrors.avroIncorrectTypeError(toFieldStr(avroPath),
+ toFieldStr(catalystPath), realDataType.catalogString, dt.catalogString, confKey.key)
+
+ case (LONG, dt: TimestampNTZType)
+ if preventReadingIncorrectType && realDataType.isInstanceOf[DayTimeIntervalType] =>
+ throw QueryCompilationErrors.avroIncorrectTypeError(toFieldStr(avroPath),
+ toFieldStr(catalystPath), realDataType.catalogString, dt.catalogString, confKey.key)
+
+ case (LONG, dt: DateType)
+ if preventReadingIncorrectType && realDataType.isInstanceOf[DayTimeIntervalType] =>
+ throw QueryCompilationErrors.avroIncorrectTypeError(toFieldStr(avroPath),
+ toFieldStr(catalystPath), realDataType.catalogString, dt.catalogString, confKey.key)
+
+ case (INT, dt: TimestampType)
+ if preventReadingIncorrectType && realDataType.isInstanceOf[YearMonthIntervalType] =>
+ throw QueryCompilationErrors.avroIncorrectTypeError(toFieldStr(avroPath),
+ toFieldStr(catalystPath), realDataType.catalogString, dt.catalogString, confKey.key)
+
+ case (INT, dt: TimestampNTZType)
+ if preventReadingIncorrectType && realDataType.isInstanceOf[YearMonthIntervalType] =>
+ throw QueryCompilationErrors.avroIncorrectTypeError(toFieldStr(avroPath),
+ toFieldStr(catalystPath), realDataType.catalogString, dt.catalogString, confKey.key)
+
+ case (INT, dt: DateType)
+ if preventReadingIncorrectType && realDataType.isInstanceOf[YearMonthIntervalType] =>
+ throw QueryCompilationErrors.avroIncorrectTypeError(toFieldStr(avroPath),
+ toFieldStr(catalystPath), realDataType.catalogString, dt.catalogString, confKey.key)
+
+ case (INT, DateType) => (updater, ordinal, value) =>
Review Comment:
It seems the logic is also changed. Are you sure where the perf regression comes from? the code structure (pattern match) or some logic?
--
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 closed pull request #42424: [SPARK-43380][SQL] Fix Avro data type conversion issues without causing performance regression
Posted by "zeruibao (via GitHub)" <gi...@apache.org>.
zeruibao closed pull request #42424: [SPARK-43380][SQL] Fix Avro data type conversion issues without causing performance regression
URL: https://github.com/apache/spark/pull/42424
--
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