PR2 (nullability bug): adding new OH SparkCatalog which enables preserving non-nullable schemas #288
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cbb330
force-pushed
the
nonnullability
branch
from
9e93d4e to
c5a45c5
Compare
cbb330
changed the title
cbb330
force-pushed
the
nonnullability
branch
7 times, most recently
from
494ee8d to
3f87c2e
Compare
cbb330
force-pushed
the
nonnullability
branch
from
3f87c2e to
43322e9
Compare
HotSushi
requested changes
Feb 27, 2025
| @@ -0,0 +1,8 @@ | |||
| package com.linkedin.openhouse.spark; | |||
|
|
|||
| public class SparkCatalog extends org.apache.iceberg.spark.SparkCatalog { | |||
There was a problem hiding this comment.
I'd strongly prefer we not introduce this layer (ie.SparkCatalog) in OSS codebase (if its a must, a better place would be li-wrapper)
Reason:
- iceberg solves this in 1.7.x (https://iceberg.apache.org/docs/1.7.1/spark-configuration/#catalog-configuration) with a configuration. Ideally we upgrade to 1.7.x and this problem is auto-solved for us.
- If we override this conf, iceberg behavior will be ignored
- It's ideal we do not touch iceberg connector code (ie: SparkCatalog, FlinkCatalog etc), its crucial OHCatalog stays minimal with its interfacing (easy for us to upgrade iceberg/spark/other dependencies)
- Adding this layer would allow other iceberg confs to be overridden which we shouldn't allow
- I'm concerned of it getting misused and thereby our forks/behavior deviating from OSS
There was a problem hiding this comment.
I also don't see this as a perfect option and appreciate the points here.
- we can't upgrade to 1.7.x because it has a dependency on higher versions of spark in spark clients
- what exactly will be ignored / changed? because we are simply extending and adding everything should be preserved
- this can codified among the contributors to not edit
| @@ -37,7 +37,7 @@ public static void configureCatalogs( | |||
| builder | |||
| .config( | |||
| String.format("spark.sql.catalog.%s", catalogName), | |||
| "org.apache.iceberg.spark.SparkCatalog") | |||
| "com.linkedin.openhouse.spark.SparkCatalog") | |||
There was a problem hiding this comment.
We'd need to change Docker code too, and all other references to this connector.
|
|
||
| // Verify id column is preserved in good catalog, not preserved in bad catalog | ||
| assertFalse(sourceSchema.apply("id").nullable(), "Source table id column should be required"); | ||
| assertTrue( | ||
| targetSchema.apply("id").nullable(), | ||
| "Target table id column required should not be preserved -- due to 1) the CTAS non-nullable preservation is off by default and 2) OS spark3.1 catalyst connector lack of support for non-null CTAS"); | ||
| targetSchemaBroken.apply("id").nullable(), |
There was a problem hiding this comment.
if both are nullable how is the SparkCatalog helping?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
problem: the OpenHouse spark catalog does not preserve non-null fields requested by user dataframes. Because of that, tables are saved with the wrong schema. This problem only affects CTAS
solution: we provide a new sparkcatalog with configuration to enable this in this PR ✅ then we then dictate all spark clients to use this spark catalog 🕐
Changes
Testing Done
Additional Information
For all the boxes checked, include additional details of the changes made in this pull request.