Draft: hoping to avoid keeping SecondaryIdQuery subPlans, that are not needed #3260
Conversation
backend/src/main/java/com/bakdata/conquery/models/query/queryplan/SecondaryIdQueryPlan.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/models/query/queryplan/SecondaryIdQueryPlan.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public final void acceptEvent(Bucket bucket, int event) { | ||
| public final boolean acceptEvent(Bucket bucket, int event) { | ||
| hit = true; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Die kann man vielleicht auch abstract machen oder wenn hit wichtig ist an doAcceptEvent delegieren
There was a problem hiding this comment.
ne die ist ja bei EventFilterNode ein no-op, deswegen hab ich sie final gemacht. das hit=true weiß ich ehrlich gesagt nicht, ob es notwendig ist, es ist ja im Endeffekt das gleiche was die LeafNode machen sollte?
| @Override | ||
| public void acceptEvent(Bucket bucket, int event) { | ||
| public boolean acceptEvent(Bucket bucket, int event) { | ||
| // Nothing to do | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Was hätte ein false hier für Auswirkungen
There was a problem hiding this comment.
ist glaube irrelevant, weil es auf die AllIdsTable geht, aber es würde signalisieren, dass das event nicht verarbeitet wurde und dami kein Zustand verursacht wurde. Und im Fall von SecondaryId, der subPlan verworfen werden kann.
| @@ -220,7 +220,7 @@ private ConceptQueryPlan createChild(QueryExecutionContext currentContext, Bucke | |||
| ConceptQueryPlan plan; | |||
|
|
|||
| // Try to reuse old child plan first before allocating new ones | |||
| if((plan = childPlanReusePool.poll()) == null) { | |||
| if ((plan = childPlanReusePool.poll()) == null) { | |||
There was a problem hiding this comment.
Warum den poll nicht schon in Zeile 220 machen?
backend/src/main/java/com/bakdata/conquery/models/query/queryplan/SecondaryIdQueryPlan.java
Outdated
Show resolved
Hide resolved
Nein, tragischer: Ein Ergebnis ist jetzt richtiger als davor :/ |
|
@thoniTUB ich habe hier ziemlich rumgefriemelt und Filter weiter aufgedröselt |
| @@ -1,5 +1,4 @@ | |||
| result,secondary,dates | |||
| a,f_a3,"{2024-06-30/2025-06-30,2026-06-30/2026-06-30}" | |||
There was a problem hiding this comment.
dass die Person drin war, war eine unfeinheit der QueryEngine.
wir haben zwei Tabellen jeweils wird verlangt den 3. Eintrag == 1 zu haben.
Tabelle mit SecondaryId:
a,f_a3,1.01,"2014-06-30/2015-06-30","a" => EventFilterNode auf SecondaryId Ebene sagt nein.
Tabelle ohne SecondaryId hat die Person definitiv einen Eintrag. Da Konzeptknoten intern verodern ist die SecondaryId f_a3 ausgegeben worden. Das war so eigentlich nie gewollt, ich hatte nur keine Ahnung wie genau ich das implementieren würde. Scheint das ganze jetzt zumindest für EventFilterNode gelöst zu haben.
There was a problem hiding this comment.
Okay, ich sehe den Fehler
...end/src/main/java/com/bakdata/conquery/models/query/filter/CollectionNotEmptyFilterNode.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/bakdata/conquery/models/query/filter/RangeFilterNode.java
Show resolved
Hide resolved
| @@ -42,7 +42,10 @@ public void nextTable(QueryExecutionContext ctx, Table currentTable) { | |||
| public void nextBlock(Bucket bucket) { | |||
| } | |||
|
|
|||
| public abstract void acceptEvent(Bucket bucket, int event); | |||
| /** | |||
| * @implSpec returning false signals that the event may be discarded. | |||
There was a problem hiding this comment.
Bitte die Doku nach bessern. So weiß ich gerade im Context vom dem PR was die intention ist, z.B.:
A return value of true means ... and can be used to ...
False means ...
| @@ -11,4 +11,6 @@ public class QueryConfig { | |||
| private ThreadPoolDefinition executionPool = new ThreadPoolDefinition(); | |||
|
|
|||
| private Duration oldQueriesTime = Duration.days(30); | |||
|
|
|||
| private int maxSecondaryIdSubPlans = 30; | |||
There was a problem hiding this comment.
Ich denke, es wäre besser, wenn man das dynamischer ein stellen kann. Sonst ist zur Erprobung und für Einzelfälle ein Neustart nötig
| @@ -217,10 +225,14 @@ private boolean isOfInterest(Bucket bucket) { | |||
| */ | |||
| private ConceptQueryPlan createChild(QueryExecutionContext currentContext, Bucket currentBucket) { | |||
|
|
|||
| ConceptQueryPlan plan; | |||
| if (childPerKey.size() >= subPlanLimit){ | |||
| throw new ConqueryError.ExecutionProcessingError();//TODO proper message | |||
There was a problem hiding this comment.
| throw new ConqueryError.ExecutionProcessingError();//TODO proper message | |
| throw new ConqueryError.ExecutionProcessingError("Insert Coin"); |
😃
| @@ -8,11 +8,10 @@ | |||
|
|
|||
| @AllArgsConstructor | |||
| @ToString | |||
| public abstract class FilterNode<FILTER_VALUE> extends EventIterating { | |||
| public abstract sealed class FilterNode<FILTER_VALUE> extends EventIterating permits EventFilterNode, AggregationResultFilterNode { | |||
There was a problem hiding this comment.
benutzen wir schon irgendwo java 17 (Preview) switch with pattern matching oder warum sealing 👀
There was a problem hiding this comment.
ne, aber ich hab die harte annahme, dass es nur EventFilter oder AggregationResultFilter gibt, in den Code gesteckt. Deswegen muss ich die Klasse sealen.
delete-merged-branch
bot
deleted the
feature/dont-create-subquery-if-not-included
branch
|
Rudimentary performance comparisons showed that the fastest queries were a bit slower than those of the reference implementation, but slowest queries were always faster than reference implementation and showed no spiking behaviour, which the reference implementation had for especially large secondaryIds. |
Makes acceptEvent return if the event has been consumed or not (i.e. if it wasn't excluded by an EventFilterNode). This is used in SecondaryIdQueryPlan to not keep QueryPlans that are not (yet) needed. Hopefully avoiding the creation of many redundant sub-QueryPlans.