[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2428: Fix Antlr Shadowing and add Integration Tests

[GitBox <gi...@apache.org>]

aokolnychyi commented on a change in pull request #2428:
URL: https://github.com/apache/iceberg/pull/2428#discussion_r612102052



##########
File path: build.gradle
##########
@@ -972,7 +972,6 @@ project(":iceberg-spark3-extensions") {
 
   dependencies {
     compileOnly project(':iceberg-spark3')
-

Review comment:
       nit: unneeded change

##########
File path: build.gradle
##########
@@ -984,7 +983,9 @@ project(":iceberg-spark3-extensions") {
     testCompile project(path: ':iceberg-spark', configuration: 'testArtifacts')
     testCompile project(path: ':iceberg-spark3', configuration: 'testArtifacts')
 
+

Review comment:
       nit: extra empty line

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala
##########
@@ -198,3 +228,71 @@ case object IcebergSqlExtensionsPostProcessor extends IcebergSqlExtensionsBaseLi
     parent.addChild(new TerminalNodeImpl(f(newToken)))
   }
 }
+
+
+/* Partially copied from Apache Spark's Parser to avoid dependency on Spark Internals */
+case object ParseErrorListener extends BaseErrorListener {

Review comment:
       nit: should we call it `IcebergParseErrorListener` to be consistent with other copied classes?

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala
##########
@@ -198,3 +228,71 @@ case object IcebergSqlExtensionsPostProcessor extends IcebergSqlExtensionsBaseLi
     parent.addChild(new TerminalNodeImpl(f(newToken)))
   }
 }
+

Review comment:
       nit: extra line

##########
File path: build.gradle
##########
@@ -1021,6 +1031,18 @@ project(':iceberg-spark3-runtime') {
     compile(project(':iceberg-nessie')) {
       exclude group: 'com.google.code.findbugs', module: 'jsr305'
     }
+
+    integrationImplementation 'org.apache.spark:spark-hive_2.12'

Review comment:
       Do we need a separate source set as test inherits other things?

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala
##########
@@ -198,3 +228,71 @@ case object IcebergSqlExtensionsPostProcessor extends IcebergSqlExtensionsBaseLi
     parent.addChild(new TerminalNodeImpl(f(newToken)))
   }
 }
+
+
+/* Partially copied from Apache Spark's Parser to avoid dependency on Spark Internals */
+case object ParseErrorListener extends BaseErrorListener {
+  override def syntaxError(
+                            recognizer: Recognizer[_, _],

Review comment:
       nit: formatting, let's use what we do in Scala that matches Spark's behavior

##########
File path: build.gradle
##########
@@ -1038,9 +1060,7 @@ project(':iceberg-spark3-runtime') {
     relocate 'com.google', 'org.apache.iceberg.shaded.com.google'
     relocate 'com.fasterxml', 'org.apache.iceberg.shaded.com.fasterxml'
     relocate 'com.github.benmanes', 'org.apache.iceberg.shaded.com.github.benmanes'
-    relocate 'org.antlr.v4.runtime', 'org.apache.iceberg.shaded.org.antlr.v4.runtime'
     relocate 'org.checkerframework', 'org.apache.iceberg.shaded.org.checkerframework'
-    relocate 'org.apache.avro', 'org.apache.iceberg.shaded.org.apache.avro'

Review comment:
       Looks like this hasn't been reverted yet.

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala
##########
@@ -150,22 +148,54 @@ class IcebergSparkSqlExtensionsParser(delegate: ParserInterface) extends ParserI
       }
     }
     catch {
-      case e: ParseException if e.command.isDefined =>
+      case e: IcebergParseException if e.command.isDefined =>
         throw e
-      case e: ParseException =>
+      case e: IcebergParseException =>
         throw e.withCommand(command)
       case e: AnalysisException =>
         val position = Origin(e.line, e.startPosition)
-        throw new ParseException(Option(command), e.message, position, position)
+        throw new IcebergParseException(Option(command), e.message, position, position)
     }
   }
+

Review comment:
       nit: extra empty lines

##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala
##########
@@ -198,3 +228,71 @@ case object IcebergSqlExtensionsPostProcessor extends IcebergSqlExtensionsBaseLi
     parent.addChild(new TerminalNodeImpl(f(newToken)))
   }
 }
+
+
+/* Partially copied from Apache Spark's Parser to avoid dependency on Spark Internals */
+case object ParseErrorListener extends BaseErrorListener {
+  override def syntaxError(
+                            recognizer: Recognizer[_, _],
+                            offendingSymbol: scala.Any,
+                            line: Int,
+                            charPositionInLine: Int,
+                            msg: String,
+                            e: RecognitionException): Unit = {
+    val (start, stop) = offendingSymbol match {
+      case token: CommonToken =>
+        val start = Origin(Some(line), Some(token.getCharPositionInLine))
+        val length = token.getStopIndex - token.getStartIndex + 1
+        val stop = Origin(Some(line), Some(token.getCharPositionInLine + length))
+        (start, stop)
+      case _ =>
+        val start = Origin(Some(line), Some(charPositionInLine))
+        (start, start)
+    }
+    throw new IcebergParseException(None, msg, start, stop)
+  }
+}
+
+/**
+ * Copied from Apache Spark
+ * A [[ParseException]] is an [[AnalysisException]] that is thrown during the parse process. It
+ * contains fields and an extended error message that make reporting and diagnosing errors easier.
+ */
+class IcebergParseException(
+                      val command: Option[String],
+                      message: String,

Review comment:
       nit: formatting

##########
File path: build.gradle
##########
@@ -1054,10 +1075,22 @@ project(':iceberg-spark3-runtime') {
     relocate 'org.apache.arrow', 'org.apache.iceberg.shaded.org.apache.arrow'
     relocate 'com.carrotsearch', 'org.apache.iceberg.shaded.com.carrotsearch'
     relocate 'org.threeten.extra', 'org.apache.iceberg.shaded.org.threeten.extra'
+    // relocate Antlr runtime and related deps to shade Iceberg specific version
+    relocate 'org.antlr.v4.runtime', 'org.apache.iceberg.shaded.org.antlr.v4.runtime'
 
     classifier null
   }
 
+  task integrationTest(type: Test) {
+    description = "Test Spark3 Runtime Jar"
+    group = "verification"
+    testClassesDirs = sourceSets.integration.output.classesDirs
+    classpath = sourceSets.integration.runtimeClasspath + files(shadowJar.archiveFile.get().asFile.path)
+    inputs.file(shadowJar.archiveFile.get().asFile.path)
+  }
+  integrationTest.dependsOn(shadowJar)

Review comment:
       nit: I think we mostly use `integrationTest.dependsOn` shadowJar like on the line below




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org