Skip to content

[#5455] feat(web): disable to update the reserved fields of table properties#5643

Merged
jerryshao merged 4 commits intoapache:mainfrom
orenccl:feat/reservedTableProp
Nov 26, 2024
Merged

[#5455] feat(web): disable to update the reserved fields of table properties#5643
jerryshao merged 4 commits intoapache:mainfrom
orenccl:feat/reservedTableProp

Conversation

@orenccl
Copy link
Collaborator

@orenccl orenccl commented Nov 21, 2024

What changes were proposed in this pull request?

Disable the creation or update of reserved fields in table properties.

Why are the changes needed?

Fix #5455.

Does this PR introduce any user-facing changes?

Users will no longer be able to create or update reserved fields in table properties.

How was this patch tested?

Tested by attempting to create or update reserved fields in table properties to ensure it is not allowed.

@orenccl orenccl force-pushed the feat/reservedTableProp branch from c7b4727 to 1f0bddf Compare November 21, 2024 15:23
Copy link
Contributor

@tengqm tengqm left a comment

Choose a reason for hiding this comment

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

LGTM

}

export const getRelationalTablePropInfo = catalog => {
if (Object.keys(relationalTablePropInfoMap).includes(catalog)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

Suggested change
if (Object.keys(relationalTablePropInfoMap).includes(catalog)) {
if (Object.hasOwn(relationalTablePropInfoMap, catalog)) {

Copy link
Collaborator Author

@orenccl orenccl Nov 22, 2024

Choose a reason for hiding this comment

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

Thank you!
This is a better approach and has already been updated.

@LauraXia123
Copy link
Contributor

image Would you mind optimizing the length of the input by the way? And disabled the immutable properties image

@orenccl orenccl marked this pull request as draft November 25, 2024 06:10
@orenccl orenccl marked this pull request as ready for review November 25, 2024 06:44
@orenccl
Copy link
Collaborator Author

orenccl commented Nov 25, 2024

Hi @LauraXia123,

I’ve updated the properties field width and the reserved/immutable column settings.

However, I noticed that some properties appear to be immutable but are not mentioned in the documentation:

  • Hive: [serde-name]
  • Iceberg: [provider, format, format-version]

I’ve marked them as immutable in the code, but I’m not certain if this is the correct approach. Could you please confirm?

@LauraXia123
Copy link
Contributor

Hi @LauraXia123,

I’ve updated the properties field width and the reserved/immutable column settings.

However, I noticed that some properties appear to be immutable but are not mentioned in the documentation:

  • Hive: [serde-name]
  • Iceberg: [provider, format, format-version]

I’ve marked them as immutable in the code, but I’m not certain if this is the correct approach. Could you please confirm?

Yes, these properties are immutable.

@orenccl
Copy link
Collaborator Author

orenccl commented Nov 25, 2024

Okay, then it's ready for review.

@LauraXia123
Copy link
Contributor

Thank you for your contributions. LGTM

@jerryshao jerryshao closed this Nov 26, 2024
@jerryshao jerryshao reopened this Nov 26, 2024
@jerryshao jerryshao merged commit 4ddd885 into apache:main Nov 26, 2024
@orenccl orenccl deleted the feat/reservedTableProp branch December 22, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Subtask] Disable to update the reserved fields of table properties

4 participants