search

delta-io / delta

An open-source storage framework that enables building a Lakehouse architecture with compute engines including Spark, PrestoDB, Flink, Trino, and Hive and APIs
https://delta.io
Apache License 2.0
7.62k stars 1.71k forks source link

[Spark] Use `condition` instead of `errorClass` in `checkError()` #3680

Closed MaxGekk closed 2 months ago

MaxGekk commented 2 months ago

Which Delta project/connector is this regarding?

Description

In the PR, I propose to use the condition parameter instead of errorClass in calls of checkError because errorClass was renamed in Spark by the PR https://github.com/apache/spark/pull/48027. This PR fixes compilation issues like:

[error]       checkError(
[error]       ^
[error] /home/runner/work/delta/delta/spark/src/test/scala/org/apache/spark/sql/delta/rowtracking/RowTrackingReadWriteSuite.scala:304:7: overloaded method checkError with alternatives:
[error]   (exception: org.apache.spark.SparkThrowable,condition: String,sqlState: Option[String],parameters: Map[String,String],context: RowTrackingReadWriteSuite.this.ExpectedContext)Unit <and>

How was this patch tested?

By compiling locally.

Does this PR introduce any user-facing changes?

No. This makes changes in tests only.

MaxGekk commented 2 months ago

@cloud-fan Could you have a look at the PR, please. It fixes compilation issues.

cloud-fan commented 2 months ago

I think Delta master branch needs to cross-compile with both Spark master and 3.5 branches. We should create a shim layer for checkError

MaxGekk commented 2 months ago

We should create a shim layer for checkError

@cloud-fan As an alternative solution, we could pass the first two arguments exception and condition (errorClass) by positions not by names. The rest args could be passed by names. I have double checked all checkError, they all starts from those two args:

  protected def checkError(
      exception: SparkThrowable,
      condition: String,
MaxGekk commented 2 months ago

@cloud-fan Compilation has been fixed. The failed tests are not related to my changes, I hope.