Query engine 1/N #1848

batiati · 2024-04-16T01:12:33Z

This PR is the first part of a multi-PR effort to implement the QueryAPI.
Evolving the existing Scans, adding the required building blocks for queries with the minimal amount of work that makes sense to be reviewed together:

1. ScanBuilder sanitization:

Implements support for id and timestamp (previously, only secondary indexes were supported);
Removes the support for range queries over secondary indexes; only equality by the prefix is supported now.

With that, we removed the unsupported use case of merging scans not sorted by timestamp from this API.

2. ScanRange type:

Range queries (e.g. >, <, >=, <=) are still possible without merging scans, just not supported by the ScanBuilder anymore.

The new type ScanRange is non-composable and acts like an adapter between the raw ScanTree (which returns the CompositeKey) and the ScanLookup (which expects a timestamp).

The implementation of the expiry of pending transfers was changed to use a ScanRange, taking advantage of the new API that allows the caller to execute inline filters during the matching.
Instead of two queries, now a single scan will suffice to fulfill the pending transfer logic. See comments for more details. No noticeable performance gain, but the code was greatly simplified.

3. Zig-zag merge join:

Implements the zig-zag merge join algorithm and all the machinery required to probe scans during the "zag" phase.

This refactors the existing K-way merge implementation to accommodate the required API for probing streams.
The most notable change is that the iterator is now initialized only during the first read, allowing "resets" when the underlying streams change due to a "zag".

There are unit and fuzz tests for the Zig-zag merge implementation.

Pending work and next steps:

Merge differences (not implemented yet).
Fuzzer for ScanBuilder and ScanMerge (most of the code present in this PR is not reachable in production. During development, I did tests and benchmarks using the existing get_account_transfers operation with a provisory modified state machine, which performed more complex AND clauses).
Query API wire protocol (payload, results, errors, pagination, batching, etc);
Client APIs (for each target language);

src/lsm/zig_zag_merge.zig

sentientwaffle · 2024-04-16T20:37:56Z

src/lsm/zig_zag_merge.zig

+                    assert(key_from_value(&value_other) == key);
+
+                    if (constants.verify) {
+                        // It's assumed that streams will produce the same value.


Wait, why are they all same value type?

(Updated later:) Ok, it is because all of the streams produce only timestamps, not objects or composite keys. Will we later use zig-zag merge iterator with Key != Value?

Will we later use zig-zag merge iterator with Key != Value?

Yes, scans produce only timestamps, and since the composite key prefix is always fixed (equality), they also take only the timestamp as the probe key, so Key == Value for merging scans.

I don't think we should enforce Key == Value, the intersection logic can be reused as a primitive.

EDIT: I just realized that this comment could be clarified.

src/lsm/zig_zag_merge.zig

src/lsm/k_way_merge.zig

src/lsm/scan_tree.zig

src/lsm/scan_builder.zig

sentientwaffle · 2024-04-22T14:18:54Z

src/lsm/zig_zag_merge.zig

+        pub fn reset(it: *ZigZagMergeIterator) void {
+            _ = it;


Is this ever called, or could it be unreachable?

It's called, so ScanMerge doesn't need to know which type of merge_iterator needs a reset.

src/state_machine.zig

src/lsm/scan_range.zig

src/state_machine.zig

src/lsm/scan_tree.zig

sentientwaffle · 2024-04-22T18:12:37Z

src/lsm/scan_tree.zig

+            // Updates the scan range depending on the direction.
+            switch (self.direction) {
+                .ascending => {
+                    if (self.key_min == key) return; // No need to move


Doesn't the ZigZagMergeIterator avoid calling probe on streams that already have the right key?
Could we assert that key_min != key here instead of returning? (Likewise for the .descending branch.)

We avoid probing a scan already in the right key, but it's possible to probe the same scan twice, first as buffered and then as drained.

This if does not check the current key, it only allows double-probing, so we don't have to track it during the iteration.

tigerbeetle/src/lsm/zig_zag_merge.zig

Lines 162 to 176 in 7a6a441

// Probing the buffered streams that did not match the key.

var probing_iterator = probing.iterator(.{ .kind = .set });

while (probing_iterator.next()) |stream_index| {

stream_probe(it.context, @intCast(stream_index), probe_key);

const key = stream_peek(it.context, @intCast(stream_index)) catch |err| {

switch (err) {

error.Empty => return null,

error.Drained => {

drained.set(stream_index);

probing.unset(stream_index);

continue;

},

}

};

src/lsm/zig_zag_merge.zig

sentientwaffle · 2024-04-22T18:24:54Z

src/lsm/scan_builder.zig

-/// ScanBuilder is a helper to create and combine scans using
-/// any of the Groove's secondary indexes.
 pub fn ScanBuilderType(


Maybe just "ScanBuilder is a helper to create and combine scans using any of the Groove's indexes."?

👍 I removed the comment while I was thinking in the TODO next line, since it won't be single-groove soon.

This is the first step to isolate scan_builder as a proper "query builder", that supports full queriable fields (id, timestamp, and secondary indexes) but also removes the support for range scans. Instead, range queries have much simpler mechanisms that do not allow merging scans by design. Since the API for range queries needed to be changed, the expiry of pending transfers was used as the main use case for the change, allowing custom logic to be inserted during the scan.

src/lsm/zig_zag_merge.zig

src/lsm/scan_range.zig

src/lsm/scan_tree.zig

sentientwaffle · 2024-04-24T20:27:25Z

Aside from those last 3 nits, this LGTM!

batiati commented Apr 16, 2024

View reviewed changes

src/lsm/zig_zag_merge.zig Outdated Show resolved Hide resolved

batiati assigned sentientwaffle Apr 16, 2024

sentientwaffle reviewed Apr 17, 2024

View reviewed changes

src/lsm/k_way_merge.zig Show resolved Hide resolved

src/lsm/scan_tree.zig Show resolved Hide resolved

src/lsm/scan_builder.zig Outdated Show resolved Hide resolved

src/lsm/scan_builder.zig Outdated Show resolved Hide resolved

src/lsm/scan_builder.zig Outdated Show resolved Hide resolved

batiati force-pushed the batiati-query-engine branch from 942caa5 to 84e49c3 Compare April 19, 2024 18:33

batiati requested a review from sentientwaffle April 19, 2024 18:47

sentientwaffle reviewed Apr 22, 2024

View reviewed changes

batiati force-pushed the batiati-query-engine branch from 5a31f3b to 778938b Compare April 23, 2024 23:27

batiati added 15 commits April 23, 2024 20:29

Zig-zag merge join

424d9f6

Remove hardcoded seed

500c80a

Fix forest_fuzz

bc9f887

Nit: move BitSet decl to the top.

62bdb53

Exclusive key might be equals the key range

d7eaaa1

Don't strip debug info when tracy is enabled

82111ca

Zig-zag review

b63beaf

ScanBuilder/ScanRange review

c6979f9

KWayMerge: document streams_count == 0

103a55e

ZigZag: Clarify comments for expected matching values.

2295a72

Address review comments

3122053

Document double probing

6a5fdba

Address review comments

d0d3fb9

Fix fuzzer

327fb90

batiati force-pushed the batiati-query-engine branch from 778938b to 327fb90 Compare April 23, 2024 23:29

batiati requested a review from sentientwaffle April 23, 2024 23:39

sentientwaffle reviewed Apr 24, 2024

View reviewed changes

src/lsm/zig_zag_merge.zig Outdated Show resolved Hide resolved

sentientwaffle reviewed Apr 24, 2024

View reviewed changes

src/lsm/scan_range.zig Outdated Show resolved Hide resolved

sentientwaffle reviewed Apr 24, 2024

View reviewed changes

src/lsm/scan_tree.zig Outdated Show resolved Hide resolved

Review

6a5ec42

batiati enabled auto-merge April 24, 2024 21:14

batiati requested a review from sentientwaffle April 25, 2024 09:29

sentientwaffle approved these changes Apr 25, 2024

View reviewed changes

batiati added this pull request to the merge queue Apr 25, 2024

Merged via the queue into main with commit 7512b69 Apr 25, 2024
26 checks passed

batiati deleted the batiati-query-engine branch April 25, 2024 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query engine 1/N #1848

Query engine 1/N #1848

batiati commented Apr 16, 2024 •

edited

sentientwaffle Apr 16, 2024

batiati Apr 19, 2024 •

edited

sentientwaffle Apr 22, 2024

batiati Apr 23, 2024

sentientwaffle Apr 22, 2024

batiati Apr 23, 2024

sentientwaffle Apr 22, 2024

batiati Apr 23, 2024

sentientwaffle commented Apr 24, 2024

	// Probing the buffered streams that did not match the key.
	var probing_iterator = probing.iterator(.{ .kind = .set });
	while (probing_iterator.next()) \|stream_index\| {
	stream_probe(it.context, @intCast(stream_index), probe_key);

	const key = stream_peek(it.context, @intCast(stream_index)) catch \|err\| {
	switch (err) {
	error.Empty => return null,
	error.Drained => {
	drained.set(stream_index);
	probing.unset(stream_index);
	continue;
	},
	}
	};

Query engine 1/N #1848

Query engine 1/N #1848

Conversation

batiati commented Apr 16, 2024 • edited

1. ScanBuilder sanitization:

2. ScanRange type:

3. Zig-zag merge join:

Pending work and next steps:

sentientwaffle Apr 16, 2024

Choose a reason for hiding this comment

batiati Apr 19, 2024 • edited

Choose a reason for hiding this comment

sentientwaffle Apr 22, 2024

Choose a reason for hiding this comment

batiati Apr 23, 2024

Choose a reason for hiding this comment

sentientwaffle Apr 22, 2024

Choose a reason for hiding this comment

batiati Apr 23, 2024

Choose a reason for hiding this comment

sentientwaffle Apr 22, 2024

Choose a reason for hiding this comment

batiati Apr 23, 2024

Choose a reason for hiding this comment

sentientwaffle commented Apr 24, 2024

batiati commented Apr 16, 2024 •

edited

batiati Apr 19, 2024 •

edited