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.

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

[Subtask] Replace ColumnDTO with ColumnImpl in test cases #1635

Closed
Tracked by #1292
mchades opened this issue Jan 22, 2024 · 3 comments · Fixed by #2487
Closed
Tracked by #1292

[Subtask] Replace ColumnDTO with ColumnImpl in test cases #1635

mchades opened this issue Jan 22, 2024 · 3 comments · Fixed by #2487
Assignees
Labels
good first issue Good for newcomers

Comments

@mchades
Copy link
Contributor

mchades commented Jan 22, 2024

Describe the subtask

Replace ColumnDTO with ColumnImpl in test cases

Parent issue

#1292

@mchades mchades self-assigned this Jan 22, 2024
@mchades mchades added the good first issue Good for newcomers label Jan 23, 2024
@jerryshao jerryshao added this to the Gravitino 0.5.0 milestone Feb 4, 2024
@zivali
Copy link
Contributor

zivali commented Mar 5, 2024

Hi @mchades,
I see in some test cases, the ColumnDTO object will be used to create TableDTO and TableCreateRequest.
e.g. in
https://github.com/datastrato/gravitino/blob/main/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalTable.java#L82-L109

For TableDTO and TableCreateRequest, should they continue to use DTO object or switch to use ColumnImpl as well?
I see in ae0054f you stayed with ColumnDTO and added a toDTOs method. So I suppose all other test cases should do the same?

@mchades
Copy link
Contributor Author

mchades commented Mar 5, 2024

Hi @zivali , below is my supplement to this issue which may answer the question you mentioned above
The criterion for determining whether to use ColumnDTO is based on:

whether the constructed Column object is needed for Java client calls to the Gravitino API.

If it is required for this purpose, then ColumnImpl should be used; otherwise, ColumnDTO can be utilized.

In the examples you provided, Column objects are used to mock the server response containing ColumnDTO, which is acceptable.
https://github.com/datastrato/gravitino/blob/ec2fd57ce77d386e1eed0b7855ef6b16fef691ac/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalTable.java#L82-L109

But the code immediately following it is faulty:
https://github.com/datastrato/gravitino/blob/ec2fd57ce77d386e1eed0b7855ef6b16fef691ac/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalTable.java#L113-L123

As the client is invoking the createTable API of Gravitino, ColumnDTO should not be passed as a parameter. We can modify the code as follows to align with our standards:

    partitionedTable =
        catalog
            .asTableCatalog()
            .createTable(
                tableId,
                fromDTOs(columns), // client should use ColumnImpl to create Table, here use fromDTOs just for code reuse
                "comment",
                Collections.emptyMap(),
                partitioning,
                DistributionDTO.NONE,
                SortOrderDTO.EMPTY_SORT);

@zivali
Copy link
Contributor

zivali commented Mar 5, 2024

I see. So the key difference lies in whether the Column object is used to call Gravitino API.
Thank you for the detailed explanation! :) I will continue to finish those test cases, which I was uncertain about.

@mchades mchades assigned zivali and mchades and unassigned mchades Mar 6, 2024
jerryshao pushed a commit that referenced this issue Mar 11, 2024
…2487)

### What changes were proposed in this pull request?
- Use `ColumnImpl` instead of `ColumnDTO` when calling gravitino API in
test cases.
- Rename variable columnDTOs to columns to avoid misunderstanding.

### Why are the changes needed?

Fix: #1635

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Existing integration tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants