Skip to content

Feature/missing hana tests#3886

Open
awildturtok wants to merge 17 commits into
developfrom
feature/missing-hana-tests
Open

Feature/missing hana tests#3886
awildturtok wants to merge 17 commits into
developfrom
feature/missing-hana-tests

Conversation

@awildturtok
Copy link
Copy Markdown
Collaborator

No description provided.

@awildturtok awildturtok requested a review from thoniTUB as a code owner May 5, 2026 14:30
Copy link
Copy Markdown
Collaborator

@thoniTUB thoniTUB left a comment

Choose a reason for hiding this comment

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

Hier werden viele Tests gelöscht, ist das korrekt?

Comment on lines +148 to +149
// Would prefer this to be `is not null`, but hana does not support that for fields
Field<String> isResolved = innerPrimaryColumn.as(IS_RESOLVED_ALIAS);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Warum kein boolean mehr und muss dann nicht auch das SQL Beispiel aus dem Kommentar angepasst werden?

Copy link
Copy Markdown
Collaborator Author

@awildturtok awildturtok May 6, 2026

Choose a reason for hiding this comment

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

steht doch im Kommentar direkt drüber? Ich hab keine Syntax / konstrukt gefunden die für Hana funktioniert (Also einfach is not null meckert hana obwohl es eigentlich eine gültige Hana Expression ist). Ich schaue mal, ob ein case-when funktioniert.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Habs über case-when gelöst. Danke für den Kommentar! 🙏

# Conflicts:
#	backend/src/test/resources/tests/aggregator/COUNT_QUARTERS_AGGREGATOR/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/COUNT_QUARTERS_AGGREGATOR/content.csv
#	backend/src/test/resources/tests/aggregator/DURATION_SUM_AGGREGATOR/DURATION_SUM.test.json
#	backend/src/test/resources/tests/aggregator/DURATION_SUM_DISTINCT_AGGREGATOR/DURATION_SUM.test.json
#	backend/src/test/resources/tests/aggregator/EVENT_DATE_AGGREGATOR/EVENT_DATE_AGGREGATOR_NO_RESTRICTION.test.json
#	backend/src/test/resources/tests/aggregator/EVENT_DATE_AGGREGATOR/EVENT_DATE_AGGREGATOR_RESTRICTION.test.json
#	backend/src/test/resources/tests/aggregator/EXISTS_AGGREGATOR/NUMBER.test.json
#	backend/src/test/resources/tests/aggregator/EXISTS_AGGREGATOR_OR/NUMBER.test.json
#	backend/src/test/resources/tests/aggregator/FIRST_AGGREGATOR/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/FLAGS_AGGREGATOR/FLAGS_AGGREGATOR.test.json
#	backend/src/test/resources/tests/aggregator/LAST_AGGREGATOR/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/MAPPED/DISTINCT/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/MAPPED/DISTINCT_MULTI/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/MAPPED/FIRST/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/MAPPED/FIRST_MULTI/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/QUARTER_AGGREGATOR/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/RANDOM_AGGREGATOR/SIMPLE_VIRTUAL_CONCEPT_Query.test.json
#	backend/src/test/resources/tests/aggregator/SUBSTRING/DISTINCT/MAPPED.test.json
#	backend/src/test/resources/tests/aggregator/SUBSTRING/DISTINCT/SIMPLE.test.json
#	backend/src/test/resources/tests/aggregator/SUBSTRING/DISTINCT_MULTI/MAPPED.test.json
#	backend/src/test/resources/tests/aggregator/SUBSTRING/FIRST/MAPPED.test.json
#	backend/src/test/resources/tests/aggregator/SUBSTRING/FIRST/SIMPLE.test.json
@awildturtok awildturtok force-pushed the feature/missing-hana-tests branch from 98afbcc to 59832b1 Compare May 18, 2026 14:27
@awildturtok awildturtok mentioned this pull request May 19, 2026
Copy link
Copy Markdown
Collaborator

@thoniTUB thoniTUB left a comment

Choose a reason for hiding this comment

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

habe nur kleinigkeiten angemerkt :)

Comment on lines +87 to +88
(this.functionProvider.getMinDateExpression()),
(this.functionProvider.getMaxDateExpression()),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(this.functionProvider.getMinDateExpression()),
(this.functionProvider.getMaxDateExpression()),
this.functionProvider.getMinDateExpression(),
this.functionProvider.getMaxDateExpression(),

Selects preprocessingSelects = Selects.builder()
.ids(tableContext.getIds())
.validityDate(tableContext.getValidityDate())
.validityDate(Optional.of(tableContext.getValidityDate()))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wird das Optional-packing weiter unten sinnvoll? Optionals sollte man eher bei Rückgabewerten nutzen nicht als member oder Argument

Comment on lines +150 to +152
// Since coalesce makes Clickhouse certain, that the field is not nullable, it will do silly stuff with it down the line:
// missing values (for example in outer-joins) will be coerced to 0 = 01-01-1970, which is clearly not correct
// Therefore we tag the values as Nullable again to make Clickhouse show some respect
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ay ay

Field<Date> lowerBound = when(validityDateRange.getStart().lessThan(restriction.getStart()), restriction.getStart())
.otherwise(validityDateRange.getStart());

Field<Date> maxDate = getMinDateExpression(); // we want to add +1 day to the end date - except when it's the max date already
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wo ist das +1 von dem im Kommentar geschrieben ist?

@Slf4j
public class ClickhouseSqlIntegrationTests extends IntegrationTests {

// SAP does not provide more than 1 image and on an update, the earlier image is deleted from dockerhub, thus latest tag is fine
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Der Kommentar trifft nicht auf CH zu oder? Ich würde hier schon ein bestimmtes Image nehmen

public class ClickhouseSqlIntegrationTests extends IntegrationTests {

// SAP does not provide more than 1 image and on an update, the earlier image is deleted from dockerhub, thus latest tag is fine
private final static DockerImageName IMAGE_TAGE = DockerImageName.parse("clickhouse/clickhouse-server");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private final static DockerImageName IMAGE_TAGE = DockerImageName.parse("clickhouse/clickhouse-server");
private final static DockerImageName IMAGE_TAG = DockerImageName.parse("clickhouse/clickhouse-server");

Comment on lines -12 to -13
6,2012-01-01,"m"
6,2012-01-01,"f" No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ist das in den Datenbanken nicht deterministisch welcher wert genommen wird?

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.

2 participants