-
Notifications
You must be signed in to change notification settings - Fork 13
Implementation of First/Last-Value select for SQL mode #3781
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
Implementation of First/Last-Value select for SQL mode #3781
Conversation
backend/src/main/java/com/bakdata/conquery/sql/conversion/dialect/HanaSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
...nd/src/main/java/com/bakdata/conquery/sql/conversion/dialect/PostgreSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
...nd/src/main/java/com/bakdata/conquery/sql/conversion/dialect/PostgreSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/sql/conversion/model/select/ValueSelectUtil.java
Show resolved
Hide resolved
backend/src/test/resources/tests/sql/selects/first/FIRST_AGGREGATOR/content.csv
Show resolved
Hide resolved
thoniTUB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cooler Ansatz mit der Window Funktion :)
backend/src/main/java/com/bakdata/conquery/sql/conversion/dialect/HanaSqlFunctionProvider.java
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/sql/conversion/dialect/HanaSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
| endDateExpression = dateRestriction.getMax().toString(); | ||
| } | ||
|
|
||
| return ColumnDateRange.of(toDateField(startDateExpression), toDateField(endDateExpression)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich habe gesehen, dass oft das Range-End exclusiv ist, ist das hier nicht so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wir behandeln alle Ranges end-inclusive, postgres konvertiert alle automatisch nach end-exclisive, was ziemlich nervig ist.
@jnsrnhld stimmt das so?
backend/src/main/java/com/bakdata/conquery/sql/conversion/dialect/HanaSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
...nd/src/main/java/com/bakdata/conquery/sql/conversion/dialect/PostgreSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/sql/conversion/model/select/ValueSelectUtil.java
Outdated
Show resolved
Hide resolved
d0abfad to
fb44820
Compare
backend/src/main/java/com/bakdata/conquery/sql/conversion/model/select/ValueSelectUtil.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/bakdata/conquery/sql/conversion/cqelement/concept/CQConceptConverter.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/sql/conversion/cqelement/concept/EventFilterCte.java
Outdated
Show resolved
Hide resolved
| endDateExpression = dateRestriction.getMax().toString(); | ||
| } | ||
|
|
||
| return ColumnDateRange.of(toDateField(startDateExpression), toDateField(endDateExpression)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wir behandeln alle Ranges end-inclusive, postgres konvertiert alle automatisch nach end-exclisive, was ziemlich nervig ist.
@jnsrnhld stimmt das so?
backend/src/main/java/com/bakdata/conquery/sql/conversion/dialect/HanaSqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/sql/conversion/model/select/SelectContext.java
Show resolved
Hide resolved
.../src/main/java/com/bakdata/conquery/sql/conversion/cqelement/concept/CQConceptConverter.java
Outdated
Show resolved
Hide resolved
...nd/src/main/java/com/bakdata/conquery/sql/conversion/cqelement/concept/PreprocessingCte.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/sql/conversion/dialect/SqlFunctionProvider.java
Outdated
Show resolved
Hide resolved
This disables CountQuartersSqlAggregator as the implementation is actually faulty
d49110a to
00056e4
Compare
b36cb76 to
6278da3
Compare
thoniTUB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nur noch die kleine Aufräumaktion (rawValidityDate->validityDateCondition/validityDateFilter). Ich hoffe das geht
| } | ||
| } | ||
|
|
||
| conditions.add(functionProvider.validityDateFilter(tableContext.getRawValidityDate())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Und kannst du den functionProvider.validityDateFilter(tableContext.getRawValidityDate()) jetzt nicht nach CQConceptConverter.java schieben und rawValidityDate aus CQTableContext.java loschen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Der Code da ist komisch vertrackt aber ich kann ihn nochmal versuchen zu verstehen, um es besser zu platzieren. So wie ich das verstehe ist der Context auch einfach etwas eine schlechte Abstraktion: Hier wird viel Vorarbeit geleistet, die dann verkettet wird. Besser wäre da drin nur den Context zu speichern und die arbeit dann in den CTEs, aber ist alles etwas merkwürdig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Habs geschafft. Musste den Code etwas verstehen, aber hab ihn dadurch echt gut aufräumen können
Fixes the bug, where null values or values without validity date would overwrite selected values.