Skip to content

Conversation

@saihemanth-cloudera
Copy link
Contributor

…eate and alter queries

What changes were proposed in this pull request?

Trimming the white spaces if provided in create and alter queries.

Why are the changes needed?

If the users are providing white spaces, the queries should execute without issues.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual testing. Will add unit tests in the next commit.

Copy link

@aturoczy aturoczy left a comment

Choose a reason for hiding this comment

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

+1 for normalizeIdentifier

@dengzhhu653
Copy link
Member

dengzhhu653 commented May 16, 2023

Looks like the failed tests related... cloud you please check them?

POSTHOOK: Input: default@hbase_table_test_1
POSTHOOK: Output: database:default
POSTHOOK: Output: default@VIEW_HBASE_TABLE_TEST_1
POSTHOOK: Lineage: VIEW_HBASE_TABLE_TEST_1.ccount SIMPLE [(hbase_table_test_1)hbase_table_test_1.FieldSchema(name:ccount, type:int, comment:), ]
Copy link
Member

Choose a reason for hiding this comment

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

nit: do you know why these are converted to lowercased, we don't change the HS2 side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a Post Executor hook, this hook pulls out the metadata info from HMS and prints it out. So this is a expected change.

+ " already exists");
}

tbl.setDbName(normalizeIdentifier(tbl.getDbName()));
Copy link
Member

Choose a reason for hiding this comment

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

The lineage info will feed into downstream project, so I'm a little nervous when the info changes.
I think we can make changes here like:

    Table tbl = new Table(req.getTable());
    tbl.setDbName(normalizeIdentifier(tbl.getDbName()));
    tbl.setTableName(normalizeIdentifier(tbl.getTableName()));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It tried to implement your suggestion but 29 tests are failing because the tests are doing assert on the Table to construct a table path or table SD or something that is expected to be modified in HMS. So I think this change is not necessary.
Having changes in qfile files that change lineage info (which is anyway expected) is far better than changing 29 tests to do something else. Thanks.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@saihemanth-cloudera saihemanth-cloudera merged commit 6b095ed into apache:master Jun 9, 2023
yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
…eate and alter queries (apache#4316) (Sai Hemanth G, Reviewed by Zhihua Deng, Attila Turoczy)
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
…eate and alter queries (apache#4316) (Sai Hemanth G, Reviewed by Zhihua Deng, Attila Turoczy)
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.

4 participants