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

Implement matching stats for SQL #3610

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

Conversation

jnsrnhld
Copy link
Collaborator

@jnsrnhld jnsrnhld commented Oct 25, 2024

No description provided.

@jnsrnhld jnsrnhld changed the title Improve SQL matching stats performance Implement matching stats for SQL Oct 25, 2024
@jnsrnhld
Copy link
Collaborator Author

Der RestartTest schlägt aktuell fehl, mir ist nicht ganz klar, warum. Alle anderen Tests laufen (lokal) durch.

@awildturtok
Copy link
Collaborator

Prefix und PrefixRange funktionieren mit dem Ansatz im Prinzip auch. Was noch nicht funktionieren wird, ist ColumnEqual und dadurch implizite Vererbung. Da musst du dann das group-by erweitern um die ColumnEqual Spalte(n).

@jnsrnhld
Copy link
Collaborator Author

@awildturtok Vielleicht liegt hier ein Missverständnis vor: Prefix und PrefixRange sind bereits in diesem Ansatz und PR enthalten. Konzepte mit ColumnEqual werden nach "altem" Ansatz berechnet. Die Grundidee dieses neuen Ansatzes ist es ja, eine gewisse Art von Konzepten gesondert zu berechnen und die anderen nach "altem" Ansatz.

Gibt es denn außer EBM (Regional) noch große Konzeptbäume, die ColumnEqual-Conditions verwenden? Ansonsten wäre das Verbesserungspotential marginal, aber der Code würde nochmal deutlich komplexer werden.

@thoniTUB
Copy link
Collaborator

Es scheint ein Jackson Problem zu geben, kommt mir auch etwas bekannt vor

2024-10-27T14:47:44.6250900Z Caused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `com.bakdata.conquery.models.datasets.concepts.MatchingStats` (no Creators, like default constructor, exist): abstract types either need to be mapped to concrete types, have custom deserializer, or contain additional type information
2024-10-27T14:47:44.6254795Z  at [Source: (GZIPInputStream); byte offset: #91] (through reference chain: com.bakdata.conquery.models.datasets.concepts.tree.TreeConcept["matchingStats"])
2024-10-27T14:47:44.6256929Z 	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
2024-10-27T14:47:44.6259162Z 	at com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1887)
2024-10-27T14:47:44.6260953Z 	at com.fasterxml.jackson.databind.DatabindContext.reportBadDefinition(DatabindContext.java:414)
2024-10-27T14:47:44.6262849Z 	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1375)
2024-10-27T14:47:44.6264874Z 	at com.fasterxml.jackson.databind.deser.AbstractDeserializer.deserialize(AbstractDeserializer.java:274)
2024-10-27T14:47:44.6266727Z 	at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
2024-10-27T14:47:44.6268885Z 	at com.fasterxml.jackson.module.blackbird.deser.SuperSonicBeanDeserializer.deserializeFromObject(SuperSonicBeanDeserializer.java:295)
2024-10-27T14:47:44.6270981Z 	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeOther(BeanDeserializer.java:220)
2024-10-27T14:47:44.6272685Z 	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:187)
2024-10-27T14:47:44.6274636Z 	at com.fasterxml.jackson.module.blackbird.deser.SuperSonicBeanDeserializer.deserialize(SuperSonicBeanDeserializer.java:116)
2024-10-27T14:47:44.6277028Z 	at com.fasterxml.jackson.databind.deser.std.StdDelegatingDeserializer.deserialize(StdDelegatingDeserializer.java:170)
2024-10-27T14:47:44.6279447Z 	at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer._deserializeTypedForId(AsPropertyTypeDeserializer.java:170)
2024-10-27T14:47:44.6281990Z 	at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromObject(AsPropertyTypeDeserializer.java:136)
2024-10-27T14:47:44.6284458Z 	at com.fasterxml.jackson.databind.deser.AbstractDeserializer.deserializeWithType(AbstractDeserializer.java:263)
2024-10-27T14:47:44.6286510Z 	at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
2024-10-27T14:47:44.6288590Z 	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
2024-10-27T14:47:44.6290620Z 	at com.fasterxml.jackson.databind.ObjectReader._bindAndClose(ObjectReader.java:2125)
2024-10-27T14:47:44.6291946Z 	at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:1501)
2024-10-27T14:47:44.6293430Z 	at com.bakdata.conquery.io.storage.xodus.stores.SerializingStore.read(SerializingStore.java:344)
2024-10-27T14:47:44.6294674Z 	... 98 common frames omitted

@awildturtok
Copy link
Collaborator

Hier fehlen CPSTypes, sonst kann Jackson nicht zwischen den 2 Klassen unterscheiden. Vlt kannst du auch den Storage parametrisieren?

@jnsrnhld jnsrnhld requested a review from awildturtok October 30, 2024 15:08
@jnsrnhld jnsrnhld force-pushed the sql/matching-stats-perf branch from c36164e to 5be35d6 Compare November 5, 2024 09:49
@jnsrnhld
Copy link
Collaborator Author

jnsrnhld commented Nov 5, 2024

@awildturtok Kannst du bitte deinen Stand nochmal pushen?

@awildturtok awildturtok force-pushed the sql/matching-stats-perf branch from 25c5bba to 213358f Compare November 5, 2024 13:03
@jnsrnhld jnsrnhld force-pushed the sql/matching-stats-perf branch from 213358f to d0b02b7 Compare November 6, 2024 08:42
@jnsrnhld jnsrnhld requested a review from awildturtok November 6, 2024 12:17
Copy link
Collaborator

@awildturtok awildturtok left a comment

Choose a reason for hiding this comment

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

Sind vor allem aufräum sachen, sieht gut aus!

@jnsrnhld
Copy link
Collaborator Author

jnsrnhld commented Nov 6, 2024

Ich warte dann auf deinen Push bevor ich ggf. etwas ändere.

additionally minor simplification in UpdateMatchingStatsSqlJob
@awildturtok
Copy link
Collaborator

@jnsrnhld habe meine Anmerkungen direkt angepasst

@jnsrnhld
Copy link
Collaborator Author

jnsrnhld commented Nov 6, 2024

@awildturtok Kannst du mich hier einmal anpingen wenn du fertig bist?

@awildturtok
Copy link
Collaborator

@jnsrnhld der zweite Fehldurchlauf war flaky, sollte jetzt passen

@jnsrnhld
Copy link
Collaborator Author

jnsrnhld commented Nov 7, 2024

Also wenn ich jetzt nichts übersehen hab hast du alle deine Anmerkungen ja selber bearbeitet. Ist von meiner Seite noch was zu tun hier?

@jnsrnhld jnsrnhld marked this pull request as ready for review November 7, 2024 07:51
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.

3 participants