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

[docs] Changed field names to uppercase in Flink create table SQL. #625

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

paul8263
Copy link
Contributor

In Oracle database if we created a table without quoting the field names, it would automatically convert the field names to uppercase. As a result, we can get the right data only if we create table in Flink with field names excatly matching cases with those defined in Oracle. Field name defined without quotes will be convert to uppercase by oracle and fieled names in SQL executed in Oracle will also be converted to uppercase. However, Flink SQL will not do it, which means usually we have to create table with uppercased field names, otherwise we would get null values.

To minify the misleading effect of the document, I suggest we change the field names in oracle-cdc.md to uppercase. By the way we can keep it consistent with those SQLs in oracle-tutorial-zh.md.

Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

Thanks @paul8263 for nice catch, I left one minor comment

Comment on lines 288 to -291
schema_name STRING METADATA FROM 'schema_name' VIRTUAL,
table_name STRING METADATA FROM 'table_name' VIRTUAL,
operation_ts TIMESTAMP_LTZ(3) METADATA FROM 'op_ts' VIRTUAL,
id INT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

The metadata column name accepts both lowercase and uppercase, could we add a note for physical column that The original oralce column name is uppercase by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @leonardBang , I will do it.

Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

LGTM

@leonardBang leonardBang merged commit cf1a72e into apache:master Nov 18, 2021
@leonardBang
Copy link
Contributor

@paul8263 Could you also open a PR for release-2.1 branch ?

@paul8263
Copy link
Contributor Author

@paul8263 Could you also open a PR for release-2.1 branch ?

I opened another PR #633

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.

None yet

2 participants