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.

Sign up for GitHub

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

Jump to bottom

KBC-1240 fix casting and nullability in Synapse CTAS queries #85

Merged
merged 2 commits into from
Dec 1, 2021
Merged

KBC-1240 fix casting and nullability in Synapse CTAS queries #85

merged 2 commits into from
Dec 1, 2021

Conversation

zajca
Copy link
Member

@zajca zajca commented Nov 29, 2021 •
edited
Loading

JIRA: https://keboola.atlassian.net/browse/KBC-2240

This PR is fixing issue when resulting table from CTAS had different nullability. When coalesce is used in CTAS Synapse will set column to nullable no matter what is set in source, if column is not null there has to be ISNULL used.

@zajca zajca marked this pull request as ready for review November 29, 2021 15:18
@zajca
Copy link
Member Author

zajca commented Nov 29, 2021 •
edited
Loading

Testy by dopadly cajk, zkusil sem tag v connection https://app.travis-ci.com/github/keboola/connection/jobs/549795525
Ale nejede exasol: https://keboolaglobal.slack.com/archives/C02988ZV06M/p1638187537062100
Connection PR: https://github.com/keboola/connection/pull/3481

@zajca zajca requested review from a team and jirkasemmler and removed request for a team November 29, 2021 15:19
jirkasemmler
jirkasemmler requested changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@jirkasemmler jirkasemmler left a comment

Choose a reason for hiding this comment

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

prosim jen dopln ten test, co udela query s ISNULL. A jestli bych te mohl poprosit priste o nejakej commit s realnym fixem a pak commit s tim prehazovanim kodu. Je v tom pak dost prace se vyznat

src/Backend/Synapse/ToFinalTable/SqlBuilder.php
} else {
// Columns with isnull are not null in CTAS
$colSql = sprintf(
'ISNULL(%s, \'\')',
Copy link
Contributor

Choose a reason for hiding this comment

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

pokud jsem to rozmotal spravne, tak tady ta vetev (a duvod celeho PR) neni otestovana v testech (vsechny destination column tam jsou nullable), tak to bych doplnil (nebo me vyved z omylu)

@zajca
Copy link
Member Author

zajca commented Dec 1, 2021

prosim jen dopln ten test, co udela query s ISNULL. A jestli bych te mohl poprosit priste o nejakej commit s realnym fixem a pak commit s tim prehazovanim kodu. Je v tom pak dost prace se vyznat

🤦 jo sorry už mě z těch Synapse usecases docela hrabe. Přidal sem testy.

jirkasemmler
jirkasemmler approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@jirkasemmler jirkasemmler left a comment

Choose a reason for hiding this comment

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

haha, prej pridam testy... -> 55 zmen. Ale jsou to vesmes jen renamy a pak hlavni test (opet asi pripomenu to, ze rozdeleni commitu by pomohlo). Ale cajk :)

@zajca zajca merged commit 29c0ff4 into master Dec 1, 2021
@zajca zajca deleted the zajca-kbc-1240 branch December 1, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jirkasemmler jirkasemmler jirkasemmler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zajca @jirkasemmler