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

multi mappable #3613

Merged
merged 14 commits into from
Nov 19, 2024
Merged

multi mappable #3613

merged 14 commits into from
Nov 19, 2024

Conversation

awildturtok
Copy link
Collaborator

No description provided.

@awildturtok
wip
245b0ee 
cleanup MAPPED tests

fix Mapped selects:
Composition was implemtented improperly
@awildturtok
fix Mapped selects:
5109600 
Composition was implemtented improperly
@awildturtok
hopefully fix IndexServiceTest
9b1fb5b
@awildturtok
hopefully fix simplifies IndexKey
188479d
@awildturtok
distinguish between MapIndex and MultiMapIndex for cleaner access
0967b31
@awildturtok awildturtok requested a review from thoniTUB October 30, 2024 15:50
awildturtok
awildturtok commented Oct 30, 2024
View reviewed changes
backend/src/main/java/com/bakdata/conquery/io/jackson/Jackson.java Outdated
.enable(Feature.ALLOW_COMMENTS)
.enable(Feature.ALLOW_UNQUOTED_CONTROL_CHARS)
.enable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE)
.disable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hier muss ich noch schauen, wie ich das mache: Der "allowMultiple" knallt sonst, wenn er nicht gesetzt ist

Copy link
Collaborator

Choose a reason for hiding this comment

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

Default Value ist keine gute Lösung?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

den defaultValue hat er nicht geschluckt ohne dass ich dieses Feature aktiviert habe, so als wäre der guard dafür vor dem anwenden des jsonProperty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich habs mir jetzt nochmal im Detal angeschaut, das muss leider so umgesetzt weerden. JsonProperty Annos machen gar nichts, außer man benutzt den JacksonAnnotationInreospector aber da gehen an vielen Stellen Sachen kaputt die nichts damit zu tun haben. Das würde unsere Serialisierung komplett über den Haufen werfen

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mhmm okay, aber wir brauchen das jetzt nur für diese eine Stelle?
Denn wir setzen doch an vielen stellen defaults für primitives, z.B. hidden, 'default' . Ist das anders?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Denn wir setzen doch an vielen stellen defaults für primitives, z.B. hidden, 'default' . Ist das anders?

Da setzen wir die immer über das field-default, ich kann natürlich auch schauen ob ich hierraus einen noArgs fall mit settern machen kann. Ist aber immer etwas unschön

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Property basiert kriege ich:

Validation failed on: class com.bakdata.conquery.models.index.MapInternToExternMapper
		- name: darf nicht leer sein
		- name: darf nicht leer sein

also das will einfach nicht :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ich kann hier nicht ganz folgen, warum name, das ist doch gar kein Primitive und warum gleich zweimal?
Ich dachte allowMultiple war der Wert der Probleme gemacht hat

Copy link
Collaborator

Choose a reason for hiding this comment

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

Da setzen wir die immer über das field-default, ich kann natürlich auch schauen ob ich hierraus einen noArgs fall mit settern machen kann. Ist aber immer etwas unschön

Finde ich nicht unschön, du kannst du ruhig einen private NoArgs mit private Settern machen. Nach Außen hast du dann nur den AllArgs/RequiredArgs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich dachte allowMultiple war der Wert der Probleme gemacht hat

SOllte es auch sein, ich hab schon wirklich alle Varianten von NoArgs+Anno+Setter probiert wenn es dann allowmultiple schluckt, failed es bei name.

...main/java/com/bakdata/conquery/models/datasets/concepts/select/connector/DistinctSelect.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/FrontendValueIndexKey.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/MapMultiIndex.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/query/resultinfo/printers/Printer.java Show resolved Hide resolved
thoniTUB
thoniTUB requested changes Nov 5, 2024
View reviewed changes
Copy link
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.

Die unterschiedlichen ResultRenderer müssen wir hier nicht beachten/testen, oder?
Bin mir gerade nicht ganz sicher

backend/src/main/java/com/bakdata/conquery/io/jackson/Jackson.java Outdated
.enable(Feature.ALLOW_COMMENTS)
.enable(Feature.ALLOW_UNQUOTED_CONTROL_CHARS)
.enable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE)
.disable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default Value ist keine gute Lösung?

.../conquery/models/datasets/concepts/select/connector/specific/MappableSingleColumnSelect.java Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/Index.java Outdated

void put(String key, Map<String, String> templateToConcrete);

int size();

void finalizer();

V external(String key, V defaultValue);

Collection<V> externalMultiple(String key, V defaultValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Müsste das nicht eigentlich so sein?:

Suggested change
Collection<V> externalMultiple(String key, V defaultValue);
Collection<V> externalMultiple(String key, Collection<V> defaultValue);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich hab die defaultValues mal in den caller gesteckt, das war eine leaky abstraktion

backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/MapInternToExternMapper.java Outdated Show resolved Hide resolved
...in/java/com/bakdata/conquery/models/query/resultinfo/printers/common/MappedMultiPrinter.java Outdated Show resolved Hide resolved
...in/java/com/bakdata/conquery/models/query/resultinfo/printers/common/MultiMappedPrinter.java Outdated Show resolved Hide resolved
backend/src/test/java/com/bakdata/conquery/service/IndexServiceTest.java Show resolved Hide resolved
...resources/tests/aggregator/DISTINCT_MAPPED_AGGREGATOR/SIMPLE_VIRTUAL_CONCEPT_Query.test.json Outdated Show resolved Hide resolved
@awildturtok awildturtok requested a review from thoniTUB November 6, 2024 12:04
thoniTUB
thoniTUB approved these changes Nov 7, 2024 •
edited
Loading
View reviewed changes
backend/src/main/java/com/bakdata/conquery/io/jackson/Jackson.java Outdated
.enable(Feature.ALLOW_COMMENTS)
.enable(Feature.ALLOW_UNQUOTED_CONTROL_CHARS)
.enable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE)
.disable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ich kann hier nicht ganz folgen, warum name, das ist doch gar kein Primitive und warum gleich zweimal?
Ich dachte allowMultiple war der Wert der Probleme gemacht hat

backend/src/main/java/com/bakdata/conquery/io/jackson/Jackson.java Outdated
.enable(Feature.ALLOW_COMMENTS)
.enable(Feature.ALLOW_UNQUOTED_CONTROL_CHARS)
.enable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE)
.disable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Da setzen wir die immer über das field-default, ich kann natürlich auch schauen ob ich hierraus einen noArgs fall mit settern machen kann. Ist aber immer etwas unschön

Finde ich nicht unschön, du kannst du ruhig einen private NoArgs mit private Settern machen. Nach Außen hast du dann nur den AllArgs/RequiredArgs

@awildturtok awildturtok merged commit 3c49cfa into develop Nov 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoniTUB thoniTUB thoniTUB 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
@awildturtok @thoniTUB