Zip Fragmentation on `take` #4747

blaginin · 2025-09-24T13:23:54Z

No description provided.

blaginin · 2025-09-24T13:24:13Z

vortex-array/src/compute/zip.rs

+          metadata: EmptyMetadata
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)
+          buffer (align=1): 28 B (0.63%)
+          buffer (align=1): 29 B (0.65%)


@onursatici this shouldn't be the case, right?

I remember you worked on something similar in #4054

codecov · 2025-09-24T13:34:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.56%. Comparing base (c7045ec) to head (c58ac57).
⚠️ Report is 2 commits behind head on develop.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

robert3005 · 2025-09-25T10:50:23Z

vortex-array/src/compute/zip.rs

        // TODO(os): add invert_mask opt and check if if_false has a kernel like:
        //           kernel.invoke(Args(if_false, if_true, mask, invert_mask = true))

+        if !if_true.is_canonical() || !if_false.is_canonical() {


what is the thing that is actually changing here? Doesn't this indicate that our default impl is bad?

It makes us use impl ZipKernel for VarBinViewVTable. Default impl is indeed doesn't handle VarBinViews properly, that's why Onur did #4054

I made the default behavior different, but i think needs to still be here, because it follows the pattern from other functions (1, 2, 3)

onursatici

I think this makes sense, but we should just make zip_impl, the default implementation when no kernels are found, to be the same as the varbinview kernel. We can then remove the varbinview kernel as well, because it will be the default for two already canonicalised arrays anyway.

we might need to branch in creating the builder to create the deduplicating builder for varbinviews, and the regular canonical builders for others

blaginin · 2025-09-25T12:05:57Z

vortex-array/src/compute/zip.rs

+    #[test]
+    fn test_varbinview_zip() {
+        let if_true = {
+            let mut builder = VarBinViewBuilder::new(
+                DType::Utf8(Nullability::NonNullable),
+                10,
+                Default::default(),
+                BufferGrowthStrategy::fixed(64 * 1024),
+            );
+            for _ in 0..100 {
+                builder.append_value("Hello");
+                builder.append_value("Hello this is a long string that won't be inlined.");


just moved this test

blaginin · 2025-09-25T12:07:34Z

vortex-array/src/builders/varbinview.rs


 impl Default for CompletedBuffers {
    fn default() -> Self {
-        Self::Default(Vec::new())


@onursatici I didn't change builder_with_capacity because IMO it would be non-intuitive if it returned something rather than the default builder. Changed it here instead - feel like we can afford Deduplicated to be the default right?

blaginin · 2025-09-25T12:08:10Z

vortex-array/src/builders/varbinview.rs


 pub enum CompletedBuffers {
-    Default(Vec<ByteBuffer>),
+    Plain(Vec<ByteBuffer>),


renamed otherwise it's strange that it is not chosen by default

codspeed-hq · 2025-09-25T12:13:26Z

CodSpeed Performance Report

Merging #4747 will not alter performance

_{Comparing db/zip-fragmentation (c58ac57) with develop (c7045ec)}

Summary

✅ 1156 untouched

onursatici

I want to turn on deduplication by default but we use it in some many places to make the switch easy: #4695
I think the default should remain the old default for now because of that

cloudflare-workers-and-pages · 2025-09-29T14:11:09Z

Deploying vortex-bench with Cloudflare Pages

Latest commit:	`172b6a6`
Status:	✅ Deploy successful!
Preview URL:	https://638614e4.vortex-93b.pages.dev
Branch Preview URL:	https://db-zip-fragmentation.vortex-93b.pages.dev

View logs

blaginin · 2025-09-29T14:11:51Z

I think the default should remain the old default for now because of that

Sure, in that case, let's handle this case specifically for VarBinViewVTable (like we do with other compute functions)? Later, once #4695 is merge, we'll handle it properly for all arrays

Signed-off-by: blaginin <dima@spiraldb.com>

AdamGS

This LGTM, IDK if anyone else wants to weigh in.

blaginin · 2025-09-29T16:23:20Z

Will discuss with @onursatici once he is back and do changes of top as needed :)

follow up on #4747 --------- Signed-off-by: blaginin <dima@spiraldb.com>

blaginin self-assigned this Sep 24, 2025

blaginin added the changelog/fix A bug fix label Sep 24, 2025

blaginin commented Sep 24, 2025

View reviewed changes

blaginin marked this pull request as ready for review September 25, 2025 10:28

blaginin requested a review from onursatici September 25, 2025 10:37

blaginin enabled auto-merge (squash) September 25, 2025 10:45

robert3005 reviewed Sep 25, 2025

View reviewed changes

onursatici reviewed Sep 25, 2025

View reviewed changes

blaginin commented Sep 25, 2025

View reviewed changes

blaginin requested a review from onursatici September 25, 2025 12:11

onursatici reviewed Sep 25, 2025

View reviewed changes

blaginin force-pushed the db/zip-fragmentation branch from f166845 to 888f13e Compare September 29, 2025 14:13

Handle zip for VarBinViewVTable

c58ac57

Signed-off-by: blaginin <dima@spiraldb.com>

blaginin force-pushed the db/zip-fragmentation branch from 172b6a6 to c58ac57 Compare September 29, 2025 14:29

blaginin requested a review from AdamGS September 29, 2025 15:12

AdamGS approved these changes Sep 29, 2025

View reviewed changes

blaginin merged commit 96d3d95 into develop Sep 29, 2025
39 checks passed

blaginin deleted the db/zip-fragmentation branch September 29, 2025 16:22

blaginin mentioned this pull request Oct 1, 2025

chore: zip struct per field #4797

Merged

blaginin added a commit that referenced this pull request Oct 1, 2025

chore: zip struct per field (#4797)

3556424

follow up on #4747 --------- Signed-off-by: blaginin <dima@spiraldb.com>

Zip Fragmentation on take #4747

Zip Fragmentation on take #4747

Uh oh!

Conversation

blaginin commented Sep 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onursatici left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blaginin Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #4747 will not alter performance

Summary

Uh oh!

onursatici left a comment

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pages bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying vortex-bench with Cloudflare Pages

Uh oh!

blaginin commented Sep 29, 2025

Uh oh!

AdamGS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blaginin commented Sep 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zip Fragmentation on `take` #4747

Zip Fragmentation on `take` #4747

codecov bot commented Sep 24, 2025 •

edited

Loading

onursatici left a comment •

edited

Loading

blaginin Sep 25, 2025 •

edited

Loading

codspeed-hq bot commented Sep 25, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 29, 2025 •

edited

Loading