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

[BUGFIX][Catalog] oracle catalog create table repeat and oracle pg null point #5517

Merged
merged 11 commits into from
Oct 13, 2023

Conversation

XiaoJiang521
Copy link
Contributor

@XiaoJiang521 XiaoJiang521 commented Sep 19, 2023

Purpose of this pull request

  1. AbstractJdbcCatalog The catalogName passed down may be null,In this case, the automatic table construction will report an error, and can not distinguish the same table and heterogeneous table construction, will become all heterogeneous table construction

  2. Previously, dialect was not added to the database sql. After dialect was added, oracle table may appear lowercase name, so determining whether the table exists will never exist, because the table name is converted to uppercase, but the actual table name is lowercase,And there will be an error building the statement

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@XiaoJiang521 XiaoJiang521 changed the title [bugfix] oracle catalog create table repeat and oracle pg null point [BUGFIX][Catalog] oracle catalog create table repeat and oracle pg null point Sep 19, 2023
@Hisoka-X
Copy link
Member

Hi, could you add some comment follow the template? To make sure reviewer to know this PR want to do what.
image

@XiaoJiang521
Copy link
Contributor Author

Hi, could you add some comment follow the template? To make sure reviewer to know this PR want to do what. image

Done

@Hisoka-X
Copy link
Member

Could you add some comment like this?
image

Shall we add some test case for this?

Copy link
Member

@whhe whhe left a comment

Choose a reason for hiding this comment

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

Good catch!

}

@Override
protected String getTableName(TablePath tablePath) {
return tablePath.getSchemaAndTableName().toUpperCase();
return tablePath.getSchemaAndTableName();
Copy link
Member

Choose a reason for hiding this comment

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

This method can be removed now as it's identical to super method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import java.util.List;
import java.util.Map;

public class JdbcOracleLowerTableIT extends AbstractJdbcIT {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to name it JdbcOracleLowercaseTableIT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok !Then I added a new part6 because e2e always reports errors

@XiaoJiang521
Copy link
Contributor Author

Could you add some comment like this? image

Shall we add some test case for this?

Added a case for oracle lowercase table creation. If the catalogname is null, go heterogeneous table creation. No case should be added
And the test case has passed

@XiaoJiang521
Copy link
Contributor Author

@whhe @ic4y Could you please review it

Copy link
Member

@whhe whhe left a comment

Choose a reason for hiding this comment

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

LGTM

@hailin0 hailin0 merged commit 103da93 into apache:dev Oct 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants