[GLUTEN-8455][VL] Port encrypted file checks to shim layer by ArnavBalyan · Pull Request #8501 · apache/gluten

ArnavBalyan · 2025-01-10T12:53:21Z

Moved encrypted parquet file checks to shim, since parquet version changes across spark versions.
Added unit tests to cover the following cases:
- No encrytpion.
- Footer encrypted, and file encrypted
- Footer plaintext, file encrypted
The behaviour continues to remain the same for any other case, and assume native scan is safe if we are unable to validate.
3.5 can be checked in similar way, but provides better metadata for encryption checking, will add the support in next PR. Currently 3.5 retains the previous behaviour of always offloading.

github-actions · 2025-01-10T12:53:37Z

github-actions · 2025-01-10T12:53:52Z

Run Gluten Clickhouse CI on x86

ArnavBalyan · 2025-01-10T12:55:15Z

cc @Yohahaha, @jackylee-ch, can you please take a look. The exception checks also works for spark 3.5 but will use the footer metadata since it's more efficient

github-actions · 2025-01-10T13:40:43Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-01-10T14:12:21Z

Run Gluten Clickhouse CI on x86

update

github-actions · 2025-01-10T15:19:56Z

Run Gluten Clickhouse CI on x86

Yohahaha

thank you for the work! left some comments.

Yohahaha · 2025-01-13T01:32:14Z

shims/spark33/src/main/scala/org/apache/gluten/utils/ExceptionUtils.scala

+ */
+package org.apache.gluten.utils
+
+object ExceptionUtils {


I think this util could be added in gluten-core

yes makes sense, do you think we can do this with 35, I just wanted to be sure once the logic is set in thanks

yes, let's move this util to common modules.

Yohahaha · 2025-01-13T01:33:43Z

shims/spark35/src/main/scala/org/apache/gluten/sql/shims/spark35/Spark35Shims.scala

+  override def isParquetFileEncrypted(
+      fileStatus: LocatedFileStatus,
+      conf: Configuration): Boolean = {
+    return false


nit: add TODO with current PR link.

Yohahaha · 2025-01-13T01:35:41Z

...-ut/spark33/src/test/scala/org/apache/spark/sql/gluten/ParquetEncryptionDetectionSuite.scala

+ *   - Ensures the file is still detected as encrypted despite the plaintext footer.
+ */
+
+class ParquetEncryptionDetectionSuite extends AnyFunSuite {


I suppose this suite can be moved to backends-velox test module after Spark35 shims check was done, right?

This test can be moved to backends-velox and tested with testWithSpecifiedSparkVersion.

And also, we can use existing parquet files instead of writing new ones each time.

Yes was planning to move it, but testWithSpecifiedSparkVersion works as well, added support thanks for the tip

xinghuayu007 · 2025-01-13T08:36:22Z

shims/spark32/src/main/scala/org/apache/gluten/sql/shims/spark32/Spark32Shims.scala

    plan.unsetTagValue(QueryPlan.OP_ID_TAG)
  }
+
+  override def isParquetFileEncrypted(


Can this function be implemented in it's parent class: shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala
?
If it has a different implement, just to override it.

The logic will be different for 3.5 shim, will check if consolidation can be done when that is added

This is a common issue in our shim code. Perhaps we should develop a better way than the current one to manage these code duplications in shim layer in future.

xinghuayu007 · 2025-01-13T08:39:40Z

shims/spark32/src/main/scala/org/apache/gluten/utils/ExceptionUtils.scala

+ */
+package org.apache.gluten.utils
+
+object ExceptionUtils {


Put it in a common class? As the code is much redundant in spark32/spark33/34/35

Minor code which needs to change with 35, updated for now

jackylee-ch · 2025-01-13T12:40:03Z

shims/spark32/src/main/scala/org/apache/gluten/sql/shims/spark32/Spark32Shims.scala

+      ParquetFileReader.readFooter(new Configuration(), fileStatus.getPath).toString
+      false
+    } catch {
+      case e: Exception if ExceptionUtils.hasCause(e, classOf[ParquetCryptoRuntimeException]) =>


Why use ExceptionUtils.hasCause instead of case _: ParquetCryptoRuntimeException ?

the exception may wrap ParquetCryptoRuntimeException, and may not directly expose it. This handles all cases thanks

jackylee-ch · 2025-01-13T12:40:18Z

shims/spark33/src/main/scala/org/apache/gluten/sql/shims/spark33/Spark33Shims.scala

+      ParquetFileReader.readFooter(new Configuration(), fileStatus.getPath).toString
+      false
+    } catch {
+      case e: Exception if ExceptionUtils.hasCause(e, classOf[ParquetCryptoRuntimeException]) =>


addressed above

jackylee-ch · 2025-01-13T12:49:00Z

...-ut/spark33/src/test/scala/org/apache/spark/sql/gluten/ParquetEncryptionDetectionSuite.scala

+ *   - Ensures the file is still detected as encrypted despite the plaintext footer.
+ */
+
+class ParquetEncryptionDetectionSuite extends AnyFunSuite {


This test can be moved to backends-velox and tested with testWithSpecifiedSparkVersion.

And also, we can use existing parquet files instead of writing new ones each time.

github-actions · 2025-01-13T22:30:24Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-01-14T05:45:04Z

Run Gluten Clickhouse CI on x86

Yohahaha

LGTM!

jackylee-ch · 2025-01-14T07:45:00Z

backends-velox/src/test/scala/org/apache/gluten/utils/ParquetEncryptionDetectionSuite.scala

+    fs.listFiles(new Path(path), false).next()
+  }
+
+//  private def withTempDir(testCode: File => Any): Unit = {


remove this since we don't need it.

jackylee-ch · 2025-01-14T07:57:54Z

backends-velox/src/test/scala/org/apache/gluten/utils/ParquetEncryptionDetectionSuite.scala

+
+  testWithSpecifiedSparkVersion(
+    "Detect encrypted Parquet without encrypted footer (plaintext footer)",
+    Array("3.2", "3.3")) {


why this test skip 3.4?

jackylee-ch · 2025-01-14T07:58:22Z

backends-velox/src/test/scala/org/apache/gluten/utils/ParquetEncryptionDetectionSuite.scala

+    }
+  }
+
+  testWithSpecifiedSparkVersion("Detect plain (unencrypted) Parquet file", Array("3.3", "3.4")) {


it is also needed for 3.2?

jackylee-ch · 2025-01-14T08:00:27Z

Basically LGTM, left few comments

github-actions · 2025-01-15T07:18:53Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-01-15T09:25:20Z

Run Gluten Clickhouse CI on x86

ArnavBalyan added 3 commits January 10, 2025 12:01

update

10c8f8c

update

01ac9c5

format

37ac738

github-actions bot added CORE works for Gluten Core VELOX labels Jan 10, 2025

update

3546f10

update

ef430a6

mend

faa6b95

update

Yohahaha reviewed Jan 13, 2025

View reviewed changes

xinghuayu007 reviewed Jan 13, 2025

View reviewed changes

jackylee-ch reviewed Jan 13, 2025

View reviewed changes

update

44befbd

update

54a212e

Yohahaha approved these changes Jan 14, 2025

View reviewed changes

jackylee-ch reviewed Jan 14, 2025

View reviewed changes

update

c78605c

update

415c09e

jackylee-ch approved these changes Jan 15, 2025

View reviewed changes

Yohahaha merged commit 67ebbc8 into apache:main Jan 16, 2025

Conversation

ArnavBalyan commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

ArnavBalyan commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

Yohahaha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArnavBalyan Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 13, 2025

Uh oh!

github-actions bot commented Jan 14, 2025

Uh oh!

Yohahaha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackylee-ch commented Jan 14, 2025

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

ArnavBalyan commented Jan 10, 2025 •

edited

Loading

ArnavBalyan Jan 13, 2025 •

edited

Loading