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

Fix/ctas oracle column definitions #237

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ParkinMike
Copy link

Fixed a bug with CTAS, Oracle Dialect will now handle null values when creating a new table as select.

@ParkinMike ParkinMike requested a review from jsimlo March 14, 2023 10:13
@jsimlo jsimlo added the bug label Mar 14, 2023
@sonarcloud
Copy link

sonarcloud bot commented Mar 23, 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 1 Code Smell

90.5% 90.5% Coverage
0.0% 0.0% Duplication

@jsimlo
Copy link
Contributor

jsimlo commented Mar 23, 2023

Alternative to #241

this.fields.clear();
Iterables.addAll(this.fields, Builder.Helper.buildAll(fields));
return castToChild(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to replaceFields()

} else {
fieldsToInclude.add(field);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if (field instanceof NullFieldLiteral || field instanceof FieldReference)

I think we want to handle all fields, including expressions, etc.
So, basically, there should be no else in there.

column("nullField", DataType.STRING, 3)
).indexes(
index("SomeTable_1").columns("otherField")
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Notice that this index is not present in the resulting SQL (which is correct and by design).
Indexes cannot be part of a CTAS, which is documented at SchemaEditor.addTableFrom(Table, SelectStatement).

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this not possible to add as tests for all dialects?
Any advantage to have this as a separate test class?

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.

2 participants