Skip to content

API: Reject unknown type for required fields and validate defaults #12302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
rdblue merged 4 commits into from
Feb 18, 2025
Merged

API: Reject unknown type for required fields and validate defaults #12302

rdblue merged 4 commits into from
Feb 18, 2025

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Feb 17, 2025

This updates NestedField to check that UnknownType is not used in a required field, which is not permitted by the spec. It also checks that default values are cast to the correct type or are rejected.

@rdblue rdblue force-pushed the unknown-fail-required-with-unknown branch from 2f28c11 to cfe64b8 Compare February 17, 2025 20:56
Fokko
Fokko approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

LGTM

This one was also on my list as well after reviewing #12211

api/src/main/java/org/apache/iceberg/types/Types.java
String.format("Invalid default value for %s: %s (must be null)", type, defaultValue));
} else if (defaultValue != null) {
return defaultValue.to(type);
Literal<?> typedDefault = defaultValue.to(type);
Copy link
Contributor

@Fokko Fokko Feb 18, 2025

Choose a reason for hiding this comment

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

Thinking of it, would a NoneLiteral make sense 🤔

Copy link
Contributor Author

@rdblue rdblue Feb 18, 2025

Choose a reason for hiding this comment

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

Right now, we make a lot of assumptions that Literal does not contain null or NaN. That way, we don't need to implement 3-value boolean logic or worry about how to handle NaN in comparisons.

If you want to have a null default, then you don't set a default value for an optional column. And this also passes through null without a problem if the Literal was null.

nastra
nastra reviewed Feb 18, 2025
View reviewed changes
@rdblue rdblue merged commit 93ef861 into apache:main Feb 18, 2025
25 checks passed
@rdblue
Copy link
Contributor Author

rdblue commented Feb 18, 2025

Thanks for the reviews, @Fokko and @nastra!

ankurbansal-tradedoubler added a commit to ankurbansal-tradedoubler/iceberg that referenced this pull request Feb 19, 2025
@ankurbansal-tradedoubler @danicafine
@manuzhang @ebyhr @dependabot @dramaticlly @nastra @tomtongue @rdblue @aihuaxu @Fokko @czy006 @bryanck @danielcweeks @pvary
update (#1)
2ffcb87 
* Site: Learn More to point to Spark QuickStart Doc (apache#12272)

* Build: Bump datamodel-code-generator from 0.27.2 to 0.28.1 (apache#12290)

* Spark 3.5: Fix job description of RewriteTablePathSparkAction (apache#12282)

* Build: Bump io.netty:netty-buffer from 4.1.117.Final to 4.1.118.Final (apache#12287)

Bumps [io.netty:netty-buffer](https://github.com/netty/netty) from 4.1.117.Final to 4.1.118.Final.
- [Commits](netty/netty@netty-4.1.117.Final...netty-4.1.118.Final)

---
updated-dependencies:
- dependency-name: io.netty:netty-buffer
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build: Bump software.amazon.awssdk:bom from 2.30.16 to 2.30.21 (apache#12286)

Bumps software.amazon.awssdk:bom from 2.30.16 to 2.30.21.

---
updated-dependencies:
- dependency-name: software.amazon.awssdk:bom
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* OpenAPI: Add overwrite option when registering a table (apache#12239)

* OpenAPI: Add optional overwrite when registering table

* simplify to overwrite

* Add the article to the description

Co-authored-by: Eduard Tudenhoefner <[email protected]>

* Update generated python as well

Signed-off-by: Hongyue Zhang <[email protected]>

* Fix import order

---------

Signed-off-by: Hongyue Zhang <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]>

* Build: Bump mkdocs-material from 9.6.3 to 9.6.4 (apache#12284)

Bumps [mkdocs-material](https://github.com/squidfunk/mkdocs-material) from 9.6.3 to 9.6.4.
- [Release notes](https://github.com/squidfunk/mkdocs-material/releases)
- [Changelog](https://github.com/squidfunk/mkdocs-material/blob/master/CHANGELOG)
- [Commits](squidfunk/mkdocs-material@9.6.3...9.6.4)

---
updated-dependencies:
- dependency-name: mkdocs-material
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Core: Fix Enabling row-lineage during Create Table (apache#12307)

* API: Reject unknown type for required fields and validate defaults (apache#12302)

* API: Fix TestInclusiveMetricsEvaluator notStartsWith tests. (apache#12303)

* Core: Add variant type support to utils and visitors (apache#11831)

* Core: Fix CI: Update tests with UnknownType from required to optional (apache#12316)

* Docs: Refactor site navigation bar (apache#12289)

* Parquet: Implement Variant readers (apache#12139)

* Docs: Add rewrite_table_path Spark Procedure (apache#12115)

* Parquet: Fix errorprone warning (apache#12324)

* Docs: Add Apache Amoro docs (apache#11966)

* Parquet: Fix performance regression in reader init (apache#12305)

* Core: Fallback to GET requests for namespace/table/view exists checks (apache#12314)

Co-authored-by: Daniel Weeks <[email protected]>

* Docs: Fix refs in Apache Amoro docs (apache#12332)

* Revert "Core: Serialize `null` when there is no current snapshot (apache#11560)" (apache#12312)

This reverts commit bf8d25f.

* Parquet: Fix performance regression in reader init (apache#12305) (apache#12329)

Co-authored-by: Bryan Keller <[email protected]>

* Checkstyle: Apply the same generic type naming rules to interfaces and classes (apache#12333)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Hongyue Zhang <[email protected]>
Co-authored-by: Danica Fine <[email protected]>
Co-authored-by: Manu Zhang <[email protected]>
Co-authored-by: Yuya Ebihara <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hongyue/Steve Zhang <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]>
Co-authored-by: Tom Tanaka <[email protected]>
Co-authored-by: Ryan Blue <[email protected]>
Co-authored-by: Aihua Xu <[email protected]>
Co-authored-by: Fokko Driesprong <[email protected]>
Co-authored-by: ConradJam <[email protected]>
Co-authored-by: Bryan Keller <[email protected]>
Co-authored-by: Daniel Weeks <[email protected]>
Co-authored-by: pvary <[email protected]>
@aihuaxu aihuaxu mentioned this pull request Feb 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nastra nastra nastra approved these changes

@Fokko Fokko Fokko approved these changes

@amogh-jahagirdar amogh-jahagirdar Awaiting requested review from amogh-jahagirdar

Assignees

No one assigned

Labels

API core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rdblue @nastra @Fokko