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

Implements endpoint to fetch query result statistics #3220

Merged
merged 30 commits into from
Dec 7, 2023

Conversation

awildturtok
Copy link
Collaborator

No description provided.

@awildturtok
Copy link
Collaborator Author

Median fehlt noch

Comment on lines 591 to 593
final Random random = new Random();
final int requiredSamples = 80; // TODO config
final BooleanSupplier samplePicker = () -> random.nextInt(managedQuery.getLastResultCount().intValue()) < requiredSamples;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay dieser SamplePicker hat mich sehr verwirrt. Ich dachte erst hier werden nur 80 Samples für die Statistik ausgewertet, aber er wird zum Sammeln der Beispielwerte genutzt.

Bitte doku.

Ich verstehe noch nicht warum du hier Random nimmst. Dadurch variiert die Menge an Samples doch von der Ergebnismenge:

  • viele Ergebnisse -> wenig Samples
  • wenig Ergebnisse -> viele Samples

Möchte man nicht lieber eine konstante/upper Bound an Samples haben, anstatt eines inversen Verhältnisses

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Es sollten immer ungefähr 80 random samples sein

Copy link
Collaborator

Choose a reason for hiding this comment

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

Stimmt, du hast recht bei der Anzahl.

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 kann das etwas ausführlicher schreiben als Kommentar, aber ich hab davor ungefähr 20min auf den Bildschirm gestarrt, irgendwie wahrscheinlichkeiten auszurechnen, bis ich gemerkt habe, dass ich das so ziemlich transparent hinkriege ohne viel hirnschmalz.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kannst du nicht von vornherein bestimmen, welche Samples gewählt werde sollen, z.B:

final int[] sampleIndexes = random.ints(0, managedQuery.getLastResultCount().intValue()).limit(requiredSamples).toArray();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dann muss ich aber den Zustand im collector tracken, das ist unschön, dazu wären die samples dann nicht mehr unabhängig je collector.

Copy link
Collaborator

Choose a reason for hiding this comment

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

DateColumnStatsCollector zählst du schon mit wieviele Werte schon gezählt wurden, das könntest du für den NumberColumnStatsCollector doch auch machen.

Was ist der Vorteil, wenn die Samples unabhängig sind? Dann würden, abgesehen von null Werten, überall gleich viele Werte existieren. Das ist doch praktisch wenn man so ein oberes Limit verbindlich festlegen kann.

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 sehe immer noch nicht den Vorteil, ich würde es erstmal so lassen, das ist eine relativ überschaubare Implementierung.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ja lass es so ich sehe nur gerade diese Nachteile:

  • Zufallsvariable beeinflusst gleichzeitig welche Zeilen gesampled werden und wieviele.
  • Anzahl der Samples ist theoretisch nur an die Menge Zeilen gebunden
  • Anzahl der Samples kann von Spalte zu Spalte variiren, was bei der Darstellung im Frontend vielleicht komisch aussieht

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anzahl der Samples kann von Spalte zu Spalte variiren, was bei der Darstellung im Frontend vielleicht komisch aussieht

das ist kein problem, die samples werden nur zur visualisierung einr kurve verwendet. Die alternative wäre die kurve serverseitig zu rendern

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.

Ein paar kleine Sachen

final IntSet entities = new IntOpenHashSet();
final AtomicInteger lines = new AtomicInteger();

final AtomicReference<CDateRange> span = new AtomicReference<>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kannst du hier ein direkt ein final CDateSet nehmen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

die DateSet aggregation dürfte deutlich langsamer sein, als nur so zu spannen? Oder was siehst du für einen Vorteil darin?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ich hattes es eher auf die AtomicReference abgesehen, denn solange der Stream nicht parallel ist reicht es doch für die closure, dass das Object CDateSet/CDateRange final ist

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ja, das stimmt, aber CDateRange ist ja immutable, deswegen muss ich das tatsächlich in einer AtomicRef austauschen.

@Getter
public class ListColumnStatsCollector<T> extends ColumnStatsCollector<Collection<T>>{

private final ColumnStatsCollector<T> underlying;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sammelt der underlying nicht potentiell zu viele Samples?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sammelt der underlying nicht potentiell zu viele Samples?

argh, du hast recht, sogar ordentlich zu viele. Das dürfte kein riesen problem sein, muss ich mir aber anschauen. Danke!

…ics/ColumnStatsCollector.java

Co-authored-by: MT <12283268+thoniTUB@users.noreply.github.com>
@awildturtok awildturtok marked this pull request as ready for review November 23, 2023 14:05
# Conflicts:
#	backend/src/main/java/com/bakdata/conquery/models/types/ResultType.java
@awildturtok
Copy link
Collaborator Author

@thoniTUB ich werde die Openapi nachliefern. Damit die biz schonmal weiter testen kann merge ich den stand so.

@awildturtok awildturtok merged commit f937c3d into develop Dec 7, 2023
6 checks passed
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.

None yet

2 participants