Skip to content

feat: add unsupported metadata column names to fallback reasons#4758

Open
hsiang-c wants to merge 5 commits into
apache:mainfrom
hsiang-c:fallback_meta_col_name
Open

feat: add unsupported metadata column names to fallback reasons#4758
hsiang-c wants to merge 5 commits into
apache:mainfrom
hsiang-c:fallback_meta_col_name

Conversation

@hsiang-c

@hsiang-c hsiang-c commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #. #4757

Rationale for this change

  • Comet falls back with [COMET: Metadata column is not supported] and users might want to know the column names that trigger the fallback.

What changes are included in this PR?

  • Add unsupported metadata columns name to fallback reasons.

How are these changes tested?

  • Unit test
  • Using Iceberg Spark
CometSort
+- CometSinkPlaceHolder
   +- CometColumnarExchange
      +- Project
         +- Filter
            +-  BatchScan spark_catalog.default.table [COMET: Metadata column _spec_id, _partition, _file, _pos is not supported]

Comment on lines +92 to +96
plan.expressions.flatMap {
case a: Attribute =>
a.isMetadataCol
case _ => false
})
if (a.isMetadataCol) Option(a.name) else Option.empty
case _ => Option.empty
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
plan.expressions.flatMap {
case a: Attribute =>
a.isMetadataCol
case _ => false
})
if (a.isMetadataCol) Option(a.name) else Option.empty
case _ => Option.empty
}
plan.expressions.collect {
case a: Attribute if a.isMetadataCol => a.name
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion, this is better.

Comment thread spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala Outdated
Comment thread spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala Outdated

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hsiang-c that makes sense to me

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI pending

@comphead comphead self-requested a review June 29, 2026 19:46
@hsiang-c hsiang-c marked this pull request as ready for review June 29, 2026 20:18
a.isMetadataCol
case _ => false
})
def hasMetadataCol(plan: SparkPlan): Seq[String] = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename the method, has prefix usually implies the Bool output

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks @comphead

@hsiang-c hsiang-c changed the title Add unsupported metadata column names to fallback reasons feat: add unsupported metadata column names to fallback reasons Jun 30, 2026

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hsiang-c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants