Skip to content
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

[#1277] feat(API): Add column attribute autoIncrement of the increment interface. #1278

Merged
merged 1 commit into from
Jan 5, 2024
Merged

[#1277] feat(API): Add column attribute autoIncrement of the increment interface. #1278

merged 1 commit into from
Jan 5, 2024

Conversation

Clearvive
Copy link
Contributor

What changes were proposed in this pull request?

Add column attribute autoIncrement of the increment interface.

Why are the changes needed?

Fix: #1277

Does this PR introduce any user-facing change?

  1. Change in user-facing APIs.

How was this patch tested?

UT

@Clearvive Clearvive added this to the Gravitino 0.4.0 milestone Dec 29, 2023
@Clearvive Clearvive self-assigned this Dec 29, 2023
mchades
mchades reviewed Dec 29, 2023
View reviewed changes
Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

Should you add an exception thrown in the server side implementation since the catalog has not been implemented yet?

api/src/main/java/com/datastrato/gravitino/rel/types/Types.java Outdated Show resolved Hide resolved
common/src/main/java/com/datastrato/gravitino/dto/rel/ColumnDTO.java Outdated Show resolved Hide resolved
common/src/main/java/com/datastrato/gravitino/dto/rel/ColumnDTO.java Outdated Show resolved Hide resolved
common/src/test/java/com/datastrato/gravitino/json/TestDTOJsonSerDe.java Show resolved Hide resolved
core/src/main/java/com/datastrato/gravitino/catalog/rel/BaseColumn.java Outdated Show resolved Hide resolved
server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTableOperations.java Show resolved Hide resolved
@Clearvive Clearvive force-pushed the 1277/add-auto-increment branch 2 times, most recently from d00c714 to 5410dd4 Compare December 29, 2023 08:55
@Clearvive Clearvive requested a review from mchades December 29, 2023 08:55
@Clearvive
Copy link
Contributor Author

Should you add an exception thrown in the server side implementation since the catalog has not been implemented yet?

It is not easy to determine whether it is set by the user in the catalog layer, as it is not a wrapper Boolean object

@Clearvive
Copy link
Contributor Author

@mchades Could you please help me take a look again?

@mchades
Copy link
Contributor

mchades commented Jan 2, 2024

Should you add an exception thrown in the server side implementation since the catalog has not been implemented yet?

It is not easy to determine whether it is set by the user in the catalog layer, as it is not a wrapper Boolean object

we can only throw an exception when autoIncrement()==true, because autoIncrement()==false is naturally supported.

@Clearvive
Copy link
Contributor Author

Should you add an exception thrown in the server side implementation since the catalog has not been implemented yet?

It is not easy to determine whether it is set by the user in the catalog layer, as it is not a wrapper Boolean object

we can only throw an exception when autoIncrement()==true, because autoIncrement()==false is naturally supported.

OK

@Clearvive Clearvive requested a review from mchades January 2, 2024 03:25
@yuqi1129 yuqi1129 mentioned this pull request Jan 2, 2024
Closed
@jerryshao
Copy link
Contributor

@mchades can you please take care of this PR? Thanks.

@mchades
Copy link
Contributor

mchades commented Jan 3, 2024

Overall, LGTM, but some comments left and IT issue seem to be addressed.

@qqqttt123
Copy link
Contributor

If this PR change user-facing interface, we should also add the related documents.

@Clearvive
Copy link
Contributor Author

If this PR change user-facing interface, we should also add the related documents.

Certainly, but now the underlying infrastructure doesn't support it, and it will throw an error. I believe once the underlying support is in place, I should add the usage instructions in the corresponding catalog documentation, rather than in this pull request.

@Clearvive Clearvive force-pushed the 1277/add-auto-increment branch from b738529 to 9b80ba8 Compare January 5, 2024 08:36
@jerryshao jerryshao changed the title [#1277] Improvement(column): Add column attribute autoIncrement of the increment interface. [#1277] Improvement(API): Add column attribute autoIncrement of the increment interface. Jan 5, 2024
@jerryshao jerryshao changed the title [#1277] Improvement(API): Add column attribute autoIncrement of the increment interface. [#1277] feat(API): Add column attribute autoIncrement of the increment interface. Jan 5, 2024
@jerryshao jerryshao merged commit 26cf610 into apache:main Jan 5, 2024
11 checks passed
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jan 8, 2024
@Clearvive @diqiu50
[apache#1277] feat(API): Add column attribute autoIncrement of the in…
913409d 
…crement interface. (apache#1278)

### What changes were proposed in this pull request?
Add column attribute autoIncrement of the increment interface.

### Why are the changes needed?
Fix: apache#1277

### Does this PR introduce _any_ user-facing change?
  1. Change in user-facing APIs.

### How was this patch tested?
UT

Co-authored-by: Clearvive <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qqqttt123 qqqttt123 qqqttt123 left review comments

@yuqi1129 yuqi1129 yuqi1129 left review comments

@jerryshao jerryshao jerryshao approved these changes

@mchades mchades Awaiting requested review from mchades

Assignees

@Clearvive Clearvive

Labels
None yet
Projects
None yet
Milestone
Gravitino 0.4.0
Development

Successfully merging this pull request may close these issues.

[Improvement] Column add auto_increment properties.
5 participants
@Clearvive @mchades @jerryshao @qqqttt123 @yuqi1129