You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@auron.apache.org by "guixiaowen (via GitHub)" <gi...@apache.org> on 2026/05/06 03:47:53 UTC
[PR] test [auron]
guixiaowen opened a new pull request, #2237:
URL: https://github.com/apache/auron/pull/2237
# Which issue does this PR close?
Closes #<issue_number>
# Rationale for this change
# What changes are included in this PR?
# Are there any user-facing changes?
# How was this patch tested?
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "guixiaowen (via GitHub)" <gi...@apache.org>.
guixiaowen commented on PR #2237:
URL: https://github.com/apache/auron/pull/2237#issuecomment-4460850873
@cxzl25 Could you please help review this code? Thanks a lot.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "guixiaowen (via GitHub)" <gi...@apache.org>.
guixiaowen commented on code in PR #2237:
URL: https://github.com/apache/auron/pull/2237#discussion_r3203086000
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -47,36 +47,33 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!scanClassName.startsWith("org.apache.iceberg.spark.source.")) {
- return None
- }
+ assert(scanClassName.startsWith("org.apache.iceberg.spark.source."), "Not iceberg scans.")
// Changelog scan carries row-level changes; not supported by native COW-only path.
- if (scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan") {
- return None
- }
+ assert(
+ !(scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan"),
+ "Not iceberg cow table.")
Review Comment:
If it returns `None`, it will not be possible to determine why the conversion failed. Therefore, it is recommended to keep the current behavior as it is.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 merged PR #2237:
URL: https://github.com/apache/auron/pull/2237
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "guixiaowen (via GitHub)" <gi...@apache.org>.
guixiaowen commented on code in PR #2237:
URL: https://github.com/apache/auron/pull/2237#discussion_r3203064028
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergConvertProvider.scala:
##########
@@ -28,18 +28,18 @@ import org.apache.auron.util.SemanticVersion
class IcebergConvertProvider extends AuronConvertProvider with Logging {
- override def isEnabled: Boolean = {
- val enabled = SparkAuronConfiguration.ENABLE_ICEBERG_SCAN.get()
- if (!enabled) {
- return false
- }
- if (!sparkCompatible) {
- logWarning(
- s"Disable Iceberg native scan: Spark $SPARK_VERSION is not supported. " +
+ override def isEnabled(exec: SparkPlan): Boolean = {
+ exec match {
+ case _: BatchScanExec =>
+ val enabled = SparkAuronConfiguration.ENABLE_ICEBERG_SCAN.get()
+ assert(enabled, "Conversion disabled: auron.enable.iceberg.scan=false.")
+ assert(
+ sparkCompatible,
s"Supported Spark versions: 3.4 to 4.0 (Iceberg ${icebergVersionOrUnknown}).")
- return false
+ enabled
+ case _ => false
Review Comment:
There is no issue here. Currently, for the Iceberg scenario, only `BatchScanExec` is supported. If more scenarios are supported in the future, they can be added 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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "guixiaowen (via GitHub)" <gi...@apache.org>.
guixiaowen commented on PR #2237:
URL: https://github.com/apache/auron/pull/2237#issuecomment-4414698973
@slfan1989 fan @yew1eb y Thank you very much for reviewing the code. The conflict has been resolved.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #2237:
URL: https://github.com/apache/auron/pull/2237#issuecomment-4414708914
@guixiaowen LGTM. @yew1eb Do you have any other opinions?
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #2237:
URL: https://github.com/apache/auron/pull/2237#issuecomment-4436439156
@guixiaowen I've quickly reviewed it and have no further comments. Counld we improve and complete the PR description?
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2237:
URL: https://github.com/apache/auron/pull/2237#discussion_r3249205373
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/AuronConverters.scala:
##########
@@ -239,23 +239,34 @@ object AuronConverters extends Logging {
case exec: ForceNativeExecutionWrapperBase => exec
case exec =>
- extConvertProviders.find(h => h.isEnabled && h.isSupported(exec)) match {
- case Some(provider) => tryConvert(exec, provider.convert)
- case None =>
- Shims.get.convertMoreSparkPlan(exec) match {
- case Some(exec) =>
- exec.setTagValue(convertibleTag, true)
- exec.setTagValue(convertStrategyTag, AlwaysConvert)
- exec
- case None =>
- if (Shims.get.isNative(exec)) { // for QueryStageInput and CustomShuffleReader
+ try {
+ extConvertProviders.find(h => h.isEnabled(exec) && h.isSupported(exec)) match {
+ case Some(provider) => tryConvert(exec, provider.convert)
+ case None =>
+ Shims.get.convertMoreSparkPlan(exec) match {
+ case Some(exec) =>
exec.setTagValue(convertibleTag, true)
exec.setTagValue(convertStrategyTag, AlwaysConvert)
exec
- } else {
- addNeverConvertReasonTag(exec)
- }
- }
+ case None =>
+ if (Shims.get.isNative(exec)) { // for QueryStageInput and CustomShuffleReader
+ exec.setTagValue(convertibleTag, true)
+ exec.setTagValue(convertStrategyTag, AlwaysConvert)
+ exec
+ } else {
+ addNeverConvertReasonTag(exec)
+ }
+ }
+ }
+ } catch {
+ case e @ (_: NotImplementedError | _: AssertionError | _: Exception) =>
+ exec.setTagValue(convertToNonNativeTag, true)
+ exec.setTagValue(convertibleTag, false)
+ exec.setTagValue(convertStrategyTag, NeverConvert)
+ exec.setTagValue(
+ neverConvertReasonTag,
+ s"${e.getMessage.replaceFirst("^assertion failed: ?", "")}")
Review Comment:
This fallback handler assumes every caught exception has a non-null message. Because the catch includes arbitrary `Exception`s, `e.getMessage` can be null, causing a `NullPointerException` while handling the fallback and preventing the plan from being returned with a reason. Use a null-safe message fallback before calling `replaceFirst`.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -48,31 +48,30 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!AuronIcebergSourceUtil.getClassOfSparkBatchQueryScan.isInstance(scan)) {
- return None
- }
+ assert(
+ AuronIcebergSourceUtil.getClassOfSparkBatchQueryScan.isInstance(scan),
+ "Not iceberg scans.")
val readSchema = scan.readSchema
val unsupportedMetadataColumns = collectUnsupportedMetadataColumns(readSchema)
// Native scan can project file-level metadata columns such as _file via partition values.
// Metadata columns that require per-row materialization (for example _pos) still fallback.
- if (unsupportedMetadataColumns.nonEmpty) {
- return None
- }
+ assert(
+ !(unsupportedMetadataColumns.nonEmpty),
+ "Has per-row materialization (for example _pos).")
val fileSchema = StructType(readSchema.fields.filterNot(isSupportedMetadataColumn))
// Supported metadata columns are materialized via per-file constant values rather than
// read from the Iceberg data file payload.
val partitionSchema = StructType(readSchema.fields.filter(isSupportedMetadataColumn))
- if (!fileSchema.fields.forall(field => NativeConverters.isTypeSupported(field.dataType))) {
- return None
- }
+ assert(
+ fileSchema.fields.forall(field => NativeConverters.isTypeSupported(field.dataType)),
+ "Has iceberg data file payload.")
Review Comment:
This assertion fires when the Iceberg data-file schema contains an unsupported type, but the recorded reason says only that there is a data file payload. That will make the new `auron.never.convert.reason` value misleading; use a message that identifies the unsupported schema/type condition.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergConvertProvider.scala:
##########
@@ -28,18 +28,18 @@ import org.apache.auron.util.SemanticVersion
class IcebergConvertProvider extends AuronConvertProvider with Logging {
- override def isEnabled: Boolean = {
- val enabled = SparkAuronConfiguration.ENABLE_ICEBERG_SCAN.get()
- if (!enabled) {
- return false
- }
- if (!sparkCompatible) {
- logWarning(
- s"Disable Iceberg native scan: Spark $SPARK_VERSION is not supported. " +
+ override def isEnabled(exec: SparkPlan): Boolean = {
+ exec match {
+ case _: BatchScanExec =>
+ val enabled = SparkAuronConfiguration.ENABLE_ICEBERG_SCAN.get()
+ assert(enabled, "Conversion disabled: auron.enable.iceberg.scan=false.")
Review Comment:
The fallback reason omits the `spark.` prefix from the SQL configuration key, while users set this option as `spark.auron.enable.iceberg.scan` and the existing fallback messages use the `spark.auron...` form. Include the full key so the Spark UI points users to the property they can actually set.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "guixiaowen (via GitHub)" <gi...@apache.org>.
guixiaowen commented on code in PR #2237:
URL: https://github.com/apache/auron/pull/2237#discussion_r3203097667
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -47,36 +47,33 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!scanClassName.startsWith("org.apache.iceberg.spark.source.")) {
- return None
- }
+ assert(scanClassName.startsWith("org.apache.iceberg.spark.source."), "Not iceberg scans.")
// Changelog scan carries row-level changes; not supported by native COW-only path.
- if (scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan") {
- return None
- }
+ assert(
+ !(scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan"),
+ "Not iceberg cow table.")
val readSchema = scan.readSchema
val unsupportedMetadataColumns = collectUnsupportedMetadataColumns(readSchema)
// Native scan can project file-level metadata columns such as _file via partition values.
// Metadata columns that require per-row materialization (for example _pos) still fallback.
- if (unsupportedMetadataColumns.nonEmpty) {
- return None
- }
+ assert(
+ !(unsupportedMetadataColumns.nonEmpty),
+ "Has per-row materialization (for example _pos).")
Review Comment:
If it returns `None`, it will not be possible to determine why the conversion failed. Therefore, it is recommended to keep the current behavior as it is.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2237:
URL: https://github.com/apache/auron/pull/2237#discussion_r3198073410
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergConvertProvider.scala:
##########
@@ -28,18 +28,18 @@ import org.apache.auron.util.SemanticVersion
class IcebergConvertProvider extends AuronConvertProvider with Logging {
- override def isEnabled: Boolean = {
- val enabled = SparkAuronConfiguration.ENABLE_ICEBERG_SCAN.get()
- if (!enabled) {
- return false
- }
- if (!sparkCompatible) {
- logWarning(
- s"Disable Iceberg native scan: Spark $SPARK_VERSION is not supported. " +
+ override def isEnabled(exec: SparkPlan): Boolean = {
+ exec match {
+ case _: BatchScanExec =>
+ val enabled = SparkAuronConfiguration.ENABLE_ICEBERG_SCAN.get()
+ assert(enabled, "Conversion disabled: auron.enable.iceberg.scan=false.")
+ assert(
+ sparkCompatible,
s"Supported Spark versions: 3.4 to 4.0 (Iceberg ${icebergVersionOrUnknown}).")
- return false
+ enabled
+ case _ => false
Review Comment:
`isEnabled` currently matches any `BatchScanExec` and uses `assert(enabled, ...)`. This means *any* V2 scan (including non-Iceberg) will throw when `spark.auron.enable.iceberg.scan=false`, and will be treated as `NeverConvert` with an Iceberg-specific reason. Consider first checking that `exec.scan` is an Iceberg scan (and returning `false` when it isn’t), and only emitting the “conversion disabled” reason for actual Iceberg scans.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -47,36 +47,33 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!scanClassName.startsWith("org.apache.iceberg.spark.source.")) {
- return None
- }
+ assert(scanClassName.startsWith("org.apache.iceberg.spark.source."), "Not iceberg scans.")
// Changelog scan carries row-level changes; not supported by native COW-only path.
- if (scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan") {
- return None
- }
+ assert(
+ !(scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan"),
+ "Not iceberg cow table.")
Review Comment:
`IcebergScanSupport.plan` now uses `assert` for “not Iceberg” / “not supported” conditions. When called from `isSupported`, this will throw for non-Iceberg `BatchScanExec` and propagate into provider selection, potentially marking unrelated scans as `NeverConvert` (or failing conversion logic elsewhere that calls `isSupported`). Recommend keeping “not applicable” paths as `None` returns, and reserving exceptions/assertions only for true invariant violations once you’ve confirmed the scan is Iceberg.
##########
thirdparty/auron-iceberg/src/test/scala/org/apache/auron/iceberg/AuronIcebergIntegrationSuite.scala:
##########
@@ -26,12 +26,43 @@ import org.apache.iceberg.deletes.PositionDelete
import org.apache.iceberg.spark.Spark3Util
import org.apache.spark.sql.{DataFrame, Row}
import org.apache.spark.sql.auron.iceberg.IcebergScanSupport
+import org.apache.spark.sql.catalyst.trees.TreeNodeTag
+import org.apache.spark.sql.execution.ExplainUtils.collectFirst
import org.apache.spark.sql.execution.datasources.v2.BatchScanExec
class AuronIcebergIntegrationSuite
extends org.apache.spark.sql.QueryTest
with BaseAuronIcebergSuite {
+ test("iceberg native scan with auron.enable.iceberg.scan=false") {
+ withTable("local.db.t2") {
+ withSQLConf("spark.auron.enable" -> "true", "spark.auron.enable.iceberg.scan" -> "false") {
+ sql("create table local.db.t2 using iceberg as select 1 as id, 'a' as v")
+ val df = sql("select * from local.db.t2")
+ df.collect()
+ val neverConvertReasonTag: TreeNodeTag[String] = TreeNodeTag("auron.never.convert.reason")
+ assert(collectFirst(df.queryExecution.executedPlan) { case batchScanExec: BatchScanExec =>
+ batchScanExec.getTagValue(neverConvertReasonTag)
+ }.get.get.equals("Conversion disabled: auron.enable.iceberg.scan=false."))
Review Comment:
The assertions here call `.get.get` on nested `Option`s; when the tag is missing this will fail with `NoSuchElementException` rather than producing a clear assertion failure. Consider asserting on the `Option` shape explicitly (e.g., `exists/contains`) so test failures report a more informative message.
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/AuronConverters.scala:
##########
@@ -239,23 +239,34 @@ object AuronConverters extends Logging {
case exec: ForceNativeExecutionWrapperBase => exec
case exec =>
- extConvertProviders.find(h => h.isEnabled && h.isSupported(exec)) match {
- case Some(provider) => tryConvert(exec, provider.convert)
- case None =>
- Shims.get.convertMoreSparkPlan(exec) match {
- case Some(exec) =>
- exec.setTagValue(convertibleTag, true)
- exec.setTagValue(convertStrategyTag, AlwaysConvert)
- exec
- case None =>
- if (Shims.get.isNative(exec)) { // for QueryStageInput and CustomShuffleReader
+ try {
+ extConvertProviders.find(h => h.isEnabled(exec) && h.isSupported(exec)) match {
+ case Some(provider) => tryConvert(exec, provider.convert)
+ case None =>
+ Shims.get.convertMoreSparkPlan(exec) match {
+ case Some(exec) =>
exec.setTagValue(convertibleTag, true)
exec.setTagValue(convertStrategyTag, AlwaysConvert)
exec
- } else {
- addNeverConvertReasonTag(exec)
- }
- }
+ case None =>
+ if (Shims.get.isNative(exec)) { // for QueryStageInput and CustomShuffleReader
+ exec.setTagValue(convertibleTag, true)
+ exec.setTagValue(convertStrategyTag, AlwaysConvert)
+ exec
+ } else {
+ addNeverConvertReasonTag(exec)
+ }
+ }
+ }
+ } catch {
+ case e @ (_: NotImplementedError | _: AssertionError | _: Exception) =>
+ exec.setTagValue(convertToNonNativeTag, true)
+ exec.setTagValue(convertibleTag, false)
+ exec.setTagValue(convertStrategyTag, NeverConvert)
+ exec.setTagValue(
+ neverConvertReasonTag,
+ s"${e.getMessage.replaceFirst("^assertion failed: ?", "")}")
Review Comment:
In the catch block, `e.getMessage.replaceFirst(...)` will throw `NullPointerException` if `getMessage` is null, masking the original failure and potentially losing the fallback reason. Use a null-safe message (e.g., `Option(e.getMessage).getOrElse(e.toString)`) before applying string cleanup.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -47,36 +47,33 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!scanClassName.startsWith("org.apache.iceberg.spark.source.")) {
- return None
- }
+ assert(scanClassName.startsWith("org.apache.iceberg.spark.source."), "Not iceberg scans.")
// Changelog scan carries row-level changes; not supported by native COW-only path.
- if (scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan") {
- return None
- }
+ assert(
+ !(scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan"),
+ "Not iceberg cow table.")
val readSchema = scan.readSchema
val unsupportedMetadataColumns = collectUnsupportedMetadataColumns(readSchema)
// Native scan can project file-level metadata columns such as _file via partition values.
// Metadata columns that require per-row materialization (for example _pos) still fallback.
- if (unsupportedMetadataColumns.nonEmpty) {
- return None
- }
+ assert(
+ !(unsupportedMetadataColumns.nonEmpty),
+ "Has per-row materialization (for example _pos).")
Review Comment:
Related to the new exception-driven fallback behavior: `assert(!(unsupportedMetadataColumns.nonEmpty), ...)` will throw for unsupported metadata columns, but if assertions are ever compiled out (e.g., `-Xdisable-assertions`) this check becomes a no-op and the native Iceberg plan could be built for unsupported schemas. Prefer explicit conditional checks (returning `None` / a failure reason) over `assert` for compatibility gating.
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/AuronConverters.scala:
##########
@@ -239,23 +239,34 @@ object AuronConverters extends Logging {
case exec: ForceNativeExecutionWrapperBase => exec
case exec =>
- extConvertProviders.find(h => h.isEnabled && h.isSupported(exec)) match {
- case Some(provider) => tryConvert(exec, provider.convert)
- case None =>
- Shims.get.convertMoreSparkPlan(exec) match {
- case Some(exec) =>
- exec.setTagValue(convertibleTag, true)
- exec.setTagValue(convertStrategyTag, AlwaysConvert)
- exec
- case None =>
- if (Shims.get.isNative(exec)) { // for QueryStageInput and CustomShuffleReader
+ try {
+ extConvertProviders.find(h => h.isEnabled(exec) && h.isSupported(exec)) match {
+ case Some(provider) => tryConvert(exec, provider.convert)
+ case None =>
+ Shims.get.convertMoreSparkPlan(exec) match {
+ case Some(exec) =>
exec.setTagValue(convertibleTag, true)
exec.setTagValue(convertStrategyTag, AlwaysConvert)
exec
- } else {
- addNeverConvertReasonTag(exec)
- }
- }
+ case None =>
+ if (Shims.get.isNative(exec)) { // for QueryStageInput and CustomShuffleReader
+ exec.setTagValue(convertibleTag, true)
+ exec.setTagValue(convertStrategyTag, AlwaysConvert)
+ exec
+ } else {
+ addNeverConvertReasonTag(exec)
+ }
+ }
+ }
Review Comment:
The `try { extConvertProviders.find(...) ... } catch { ... }` wraps the entire provider selection + shim conversion. If a provider throws (e.g., due to `assert`), this catches and returns early, skipping `Shims.get.convertMoreSparkPlan` and any later logic that might still successfully convert/handle the plan. Consider making providers return `false` (not throw) when they are not applicable, or isolating exception handling per-provider so one bad provider doesn’t block other conversion paths.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -47,36 +47,33 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!scanClassName.startsWith("org.apache.iceberg.spark.source.")) {
- return None
- }
+ assert(scanClassName.startsWith("org.apache.iceberg.spark.source."), "Not iceberg scans.")
// Changelog scan carries row-level changes; not supported by native COW-only path.
- if (scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan") {
- return None
- }
+ assert(
+ !(scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan"),
+ "Not iceberg cow table.")
val readSchema = scan.readSchema
val unsupportedMetadataColumns = collectUnsupportedMetadataColumns(readSchema)
// Native scan can project file-level metadata columns such as _file via partition values.
// Metadata columns that require per-row materialization (for example _pos) still fallback.
- if (unsupportedMetadataColumns.nonEmpty) {
- return None
- }
+ assert(
+ !(unsupportedMetadataColumns.nonEmpty),
+ "Has per-row materialization (for example _pos).")
Review Comment:
These assertions use negated conditions like `assert(!(unsupportedMetadataColumns.nonEmpty), ...)`, which makes the intent harder to read and maintain. Prefer direct predicates (`unsupportedMetadataColumns.isEmpty`, `formats.size <= 1`, etc.), especially since these messages are user-facing via `auron.never.convert.reason`.
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AURON #2238] Add support for auron.never.convert.reason in Iceberg scan scenarios [auron]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #2237:
URL: https://github.com/apache/auron/pull/2237#discussion_r3215982012
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -198,9 +192,9 @@ object IcebergScanSupport extends Logging {
private def icebergPartition(partition: InputPartition): Option[IcebergPartitionView] = {
val className = partition.getClass.getName
// Only accept Iceberg SparkInputPartition to access task groups.
- if (!AuronIcebergSourceUtil.getClassOfSparkInputPartition().isInstance(partition)) {
- return None
- }
+ assert(
Review Comment:
```
// Line 51: no parentheses
AuronIcebergSourceUtil.getClassOfSparkBatchQueryScan.isInstance(scan)
// Line 195: with parentheses
AuronIcebergSourceUtil.getClassOfSparkInputPartition().isInstance(partition)
```
Problem: Inconsistent style - one method call has parentheses, the other doesn't.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -47,36 +47,33 @@ object IcebergScanSupport extends Logging {
val scan = exec.scan
val scanClassName = scan.getClass.getName
// Only handle Iceberg scans; other sources must stay on Spark's path.
- if (!scanClassName.startsWith("org.apache.iceberg.spark.source.")) {
- return None
- }
+ assert(scanClassName.startsWith("org.apache.iceberg.spark.source."), "Not iceberg scans.")
// Changelog scan carries row-level changes; not supported by native COW-only path.
- if (scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan") {
- return None
- }
+ assert(
+ !(scanClassName == "org.apache.iceberg.spark.source.SparkChangelogScan"),
+ "Not iceberg cow table.")
Review Comment:
```
// Before
assert(!AuronIcebergSourceUtil.getClassOfSparkBatchQueryScan.isInstance(scan),
"Not iceberg scans.")
// After
assert(AuronIcebergSourceUtil.getClassOfSparkBatchQueryScan.isInstance(scan),
"Not iceberg scans.")
```
- Original logic: `!isInstance` → asserts failure when it's NOT an Iceberg scan (already confusing)
- New logic: `isInstance` → asserts failure when it IS an Iceberg scan
- **Error message unchanged**: "Not iceberg scans." contradicts the assertion condition
- Code comment states: "Only handle Iceberg scans", but both old and new logic are confusing
--
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: commits-unsubscribe@auron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org