German strings, attempt 3 #1082

a10y · 2024-10-17T20:19:56Z

Implementing VarBinView as our canonical string type. Concretely, this involves

Changing Canonical variant from VarBin to VarBinView and updating all of those method names accordingly
impl IntoCanonical for VarBinArray that involves doing a single-pass construction of views
Changing the Arrow type from Utf8/Binary to Utf8View/BinaryView and propagating that in Datafusion and Python bindings
Changing how ChunkedArray canonicalize works to repack views instead of repacking arrays

Some caveats

Currently in varbin -> varbinview, we reuse the bytes heap backing the varbin as the buffer for varbinview. This means that if the array contains > 2GiB of string data, we will fail currently. I can work on the rollover logic, it's just going to make the PR larger. Alternatively we can say that having chunks > 2GiB is ok, but then it fails converting to Arrow and we just say you can't have a RecordBatch of strings > 2GiB in size. But I don't think that's a great API.
This change has little effect on TPC-H benchmarks. Most queries are the same, some are slightly faster and some are ~10-30% slower. Previously canonicalizing FSST was pretty trivial and didn't require looping over the values, now with the additional view construction it adds overhead to get all bytes and construct the views

…ngs-take3

vortex-array/src/array/constant/canonical.rs

vortex-sampling-compressor/tests/smoketest.rs

a10y · 2024-10-17T21:34:34Z

vortex-array/src/array/chunked/canonical.rs

+fn pack_views(
+    chunks: &[Array],
+    dtype: &DType,
+    validity: Validity,
+) -> VortexResult<VarBinViewArray> {
+    let mut views = Vec::new();


this whole section was copied over from the last 2 PRs

lwwmanning · 2024-10-17T21:50:01Z

🇩🇪

the transmute was causing issues with deallocation.

a10y · 2024-10-18T02:12:34Z

encodings/dict/src/compress.rs

+        values
+            .into_canonical()
+            .vortex_expect("VarBin to canonical")
+            .into_varbinview()
+            .vortex_expect("VarBinView"),


preferably do this in a single-pass

we should be able to "fast-path" build a VarBinView since we know that the strings in values are unique (doesn't need to be in this PR)

robert3005 · 2024-10-18T10:37:48Z

Failed example:
    array.to_pandas()
Exception raised:
    Traceback (most recent call last):
      File "/home/runner/.rye/py/cpython@3.11.9/lib/python3.11/doctest.py", line 1355, in __run
        exec(compile(example.source, filename, "single",
      File "<doctest default[1]>", line 1, in <module>
        array.to_pandas()
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/core/frame.py", line 1214, in __repr__
        return self.to_string(**repr_params)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/util/_decorators.py", line 333, in wrapper
        return func(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/core/frame.py", line 1394, in to_string
        return fmt.DataFrameRenderer(formatter).to_string(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 962, in to_string
        string = string_formatter.to_string()
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/string.py", line 29, in to_string
        text = self._get_string_representation()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/string.py", line 44, in _get_string_representation
        strcols = self._get_strcols()
                  ^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/string.py", line 35, in _get_strcols
        strcols = self.fmt.get_strcols()
                  ^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 476, in get_strcols
        strcols = self._get_strcols_without_index()
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 729, in _get_strcols_without_index
        str_columns = self._get_formatted_column_labels(self.tr_frame)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 809, in _get_formatted_column_labels
        need_leadsp = dict(zip(fmt_columns, map(is_numeric_dtype, dtypes)))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/core/dtypes/common.py", line 1119, in is_numeric_dtype
        return _is_dtype_type(
               ^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/core/dtypes/common.py", line 1468, in _is_dtype_type
        tipo = pandas_dtype(arr_or_dtype).type
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/vortex/vortex/.venv/lib/python3.11/site-packages/pandas/core/dtypes/dtypes.py", line 2169, in type
        raise NotImplementedError(pa_type)
    NotImplementedError: string_view

too ambitious

robert3005 · 2024-10-18T10:45:05Z

Seems this might be just an issue with to_string or parts of the codepath when you need to get pandas type out of arrow type

a10y · 2024-10-18T12:58:20Z

Yea, so it works fine when I bind it to a variable, and then print the variable

>>> df = names.to_arrow_table().to_pandas()
>>> df
  name
0    a

But not if I let it log to the repl

>>> names.to_arrow_table().to_pandas()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/core/frame.py", line 1214, in __repr__
    return self.to_string(**repr_params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/util/_decorators.py", line 333, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/core/frame.py", line 1394, in to_string
    return fmt.DataFrameRenderer(formatter).to_string(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 962, in to_string
    string = string_formatter.to_string()
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/string.py", line 29, in to_string
    text = self._get_string_representation()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/string.py", line 44, in _get_string_representation
    strcols = self._get_strcols()
              ^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/string.py", line 35, in _get_strcols
    strcols = self.fmt.get_strcols()
              ^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 476, in get_strcols
    strcols = self._get_strcols_without_index()
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 729, in _get_strcols_without_index
    str_columns = self._get_formatted_column_labels(self.tr_frame)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/io/formats/format.py", line 809, in _get_formatted_column_labels
    need_leadsp = dict(zip(fmt_columns, map(is_numeric_dtype, dtypes)))
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/core/dtypes/common.py", line 1119, in is_numeric_dtype
    return _is_dtype_type(
           ^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/core/dtypes/common.py", line 1468, in _is_dtype_type
    tipo = pandas_dtype(arr_or_dtype).type
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/Code/vortex/.venv/lib/python3.11/site-packages/pandas/core/dtypes/dtypes.py", line 2169, in type
    raise NotImplementedError(pa_type)
NotImplementedError: string_view

I'll file an issue with pandas, in the meantime just fixed up the example to assign to variable then log

danking · 2024-10-18T13:01:27Z

Wat

robert3005 · 2024-10-18T13:16:56Z

this does look like tostring being wrong

a10y · 2024-10-18T13:32:59Z

Filed: pandas-dev/pandas#60068

github-actions

Vortex bytes_at

Benchmark suite	Current: `f524f8f`	Previous: `acdd46b`	Ratio
`bytes_at/array_data`	`711.5127204052014` ns (`1.031384300668492`)	`688.4294123541362` ns (`1.9603893620694635`)	`1.03`
`bytes_at/array_view`	`179.63084933526906` ns (`0.3510182229461378`)	`878.9548024570145` ns (`2.514577200262977`)	`0.20`

This comment was automatically generated by workflow using github-action-benchmark.

vortex-scalar/src/arrow.rs

a10y · 2024-10-22T14:00:47Z

Putting back into draft to make a couple of small changes. Namely I think we can just use Arrow casting to canonicalize VarBin -> View, it looks ~10x faster than what I have based on microbenchmarks.

a10y · 2024-10-22T14:13:51Z

Alright that seems to have closed the perf gap for the scan-heavy queries.

a10y · 2024-10-22T14:31:06Z

vortex-array/src/array/varbinview/compute.rs

-        let array_arrow = self.clone().into_array().into_canonical()?.into_arrow()?;
+        let array_ref = varbinview_as_arrow(self);


this one line made a ~20x difference in take performance.

take_strings benchmark was 40µs with old version, mostly due to the try_from.

This brings it down to 2µs

vortex-array/src/canonical.rs

vortex-array/src/array/varbin/arrow.rs

lwwmanning · 2024-10-22T15:15:27Z

vortex-array/Cargo.toml

+
+[[bench]]
+name = "take_strings"
+harness = false


nit: newline EOF

lwwmanning · 2024-10-22T15:18:49Z

vortex-array/src/array/chunked/canonical.rs

+    dtype: &DType,
+    validity: Validity,
+) -> VortexResult<VarBinViewArray> {
+    let mut views: Vec<u128> = Vec::new();


small optimization: chunks.iter().map(|a| a.len()).sum() is the views capacity

lwwmanning · 2024-10-22T15:21:03Z

vortex-array/src/array/constant/canonical.rs

+            // Create a view to hold the scalar bytes.
+            // If the scalar cannot be inlined, allocate a single buffer large enough to hold it.
+            let view: u128 = make_view(scalar_bytes, 0, 0);
+            let mut buffers = Vec::with_capacity(1);


conversely, better for this to be Vec::new() because current version will always allocate even if not needed

I was sort of betting most constant strings will be > 12bytes but actually, most constant strings are probably very short. will change

lwwmanning · 2024-10-22T15:21:20Z

vortex-array/src/array/constant/canonical.rs

+            }
+
+            // Clone our constant view `len` times.
+            // TODO(aduffy): switch this out for a ConstantArray once we support u128 scalars in Vortex.


file an issue?

lwwmanning · 2024-10-22T15:24:22Z

vortex-array/src/array/varbin/arrow.rs

+    let offsets = match offsets.ptype() {
+        PType::I32 | PType::I64 => offsets,
+        PType::U64 => offsets.reinterpret_cast(PType::I64),
+        PType::U32 => offsets.reinterpret_cast(PType::I32),


we should call try_cast here, which IIRC will reinterpret cast after checking that offsets max is <= I64::MAX. (if not, that's what it should do)

This is just copying existing code from canonical module into varbin module

but also, yes

vortex-array/src/array/varbinview/mod.rs

lwwmanning · 2024-10-22T15:38:41Z

vortex-array/src/array/varbinview/mod.rs

    }

+    // TODO(aduffy): do we really need to do this with copying?


+1, can we just return a VortexResult<&[u8]> instead, and the caller can copy if it wants owned bytes?

it's a bit awkward with the inlined views (b/c you can't return a reference to a u128 that is on the stack)

can return a Buffer that points to the underlying bytes within the inlined view I suppose

FWIW the only place this is used currently is ScalarAt which would already just take the reference and convert it to an owned bytes anyway

lwwmanning · 2024-10-22T15:43:17Z

vortex-array/src/canonical.rs

-/// Binary and String views are a new, better encoding format for nearly all use-cases. For now,
-/// because DataFusion does not include pervasive support for compute over StringView, we opt to use
-/// the [`VarBinArray`] as the canonical encoding (which corresponds to the Arrow `BinaryViewArray`).
+/// Binary and String views, also known as "German strings" are a better encoding format for


vortex-array/src/lib.rs

lwwmanning · 2024-10-22T15:46:40Z

encodings/dict/src/compress.rs

+        values
+            .into_canonical()
+            .vortex_expect("VarBin to canonical")
+            .into_varbinview()
+            .vortex_expect("VarBinView"),


we should be able to "fast-path" build a VarBinView since we know that the strings in values are unique (doesn't need to be in this PR)

a10y · 2024-10-22T16:31:21Z

@lwwmanning I'd prefer to do the DictArray thing in a followup, I think that also gets easier with u128 support, filed #1111 as an independent but related issue

…ngs-take3

lwwmanning

🇩🇪 🚀 🥳

a10y added 3 commits October 17, 2024 11:13

part 1

7838f7a

monster krabbie patty??!

09118d5

Merge remote-tracking branch 'origin/develop' into aduffy/german-stri…

233d7e8

…ngs-take3

a10y commented Oct 17, 2024

View reviewed changes

vortex-array/src/array/constant/canonical.rs Outdated Show resolved Hide resolved

a10y commented Oct 17, 2024

View reviewed changes

vortex-array/src/array/constant/canonical.rs Outdated Show resolved Hide resolved

a10y changed the title ~~German strings, part 3~~ German strings, attempt 3 Oct 17, 2024

pass all tests

a6335b5

a10y commented Oct 17, 2024

View reviewed changes

vortex-sampling-compressor/tests/smoketest.rs Show resolved Hide resolved

rye run ruff raggy rokay

6f9adfb

a10y commented Oct 17, 2024

View reviewed changes

a10y added 4 commits October 17, 2024 21:34

fix issue caught by miri

04d8ee1

the transmute was causing issues with deallocation.

cargofmt

e3659bb

canonicalize test

abb1ee6

fix clippy

1b143da

a10y commented Oct 18, 2024

View reviewed changes

a10y added 3 commits October 17, 2024 22:13

delete unusedcode for now

548f63c

enable FilterFn on VarBin -> fix issue with FSST filter

3c1af26

fix a bunch of python doctest

a636365

fix evermore doctests

f524f8f

a10y added the benchmark Run benchmarks on this branch label Oct 18, 2024

github-actions bot removed benchmark Run benchmarks on this branch labels Oct 18, 2024

github-actions bot reviewed Oct 18, 2024

View reviewed changes

a10y marked this pull request as ready for review October 22, 2024 02:57

enforce buffer_lens on VarBinView construction

c0e85ec

a10y commented Oct 22, 2024

View reviewed changes

vortex-scalar/src/arrow.rs Outdated Show resolved Hide resolved

move varbin_datum and varbin_to_arrow to separate module

b8fcac0

a10y marked this pull request as draft October 22, 2024 13:59

just use arrow_cast for canonicalizing VarBin

656fde2

benchmark for take, make vbv take faster

52eebe5

a10y commented Oct 22, 2024

View reviewed changes

a10y marked this pull request as ready for review October 22, 2024 14:56

AdamGS reviewed Oct 22, 2024

View reviewed changes

vortex-array/src/canonical.rs Outdated Show resolved Hide resolved

AdamGS reviewed Oct 22, 2024

View reviewed changes

vortex-array/src/canonical.rs Outdated Show resolved Hide resolved

AdamGS reviewed Oct 22, 2024

View reviewed changes

vortex-array/src/array/varbin/arrow.rs Outdated Show resolved Hide resolved

some comments

2f45c46

lwwmanning reviewed Oct 22, 2024

View reviewed changes

some comments

1451b8f

a10y added 2 commits October 22, 2024 12:31

comments

f7a42aa

Merge remote-tracking branch 'origin/develop' into aduffy/german-stri…

980fd9e

…ngs-take3

lwwmanning approved these changes Oct 22, 2024

View reviewed changes

a10y added 4 commits October 22, 2024 13:08

fix FSST compare semantic when constant is null

e12d552

use constant

ed7ba78

fix up py tests

56bb774

gakgjsedrkghskerjghsejkrghjk

8adc8a3

a10y enabled auto-merge (squash) October 22, 2024 18:54

a10y merged commit b9335aa into develop Oct 22, 2024
5 checks passed

a10y deleted the aduffy/german-strings-take3 branch October 22, 2024 19:07

lwwmanning mentioned this pull request Oct 23, 2024

Re-evaluate values canonicalization in DictArray::into_canonical #1041

Closed

robert3005 mentioned this pull request Nov 5, 2024

Add back support for flattening VarBinView #392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

German strings, attempt 3 #1082

German strings, attempt 3 #1082

a10y commented Oct 17, 2024 •

edited

Loading

a10y Oct 17, 2024

lwwmanning commented Oct 17, 2024

a10y Oct 18, 2024

lwwmanning Oct 22, 2024

robert3005 commented Oct 18, 2024

robert3005 commented Oct 18, 2024

a10y commented Oct 18, 2024 •

edited

Loading

danking commented Oct 18, 2024

robert3005 commented Oct 18, 2024

a10y commented Oct 18, 2024

github-actions bot left a comment

a10y commented Oct 22, 2024

a10y commented Oct 22, 2024

a10y Oct 22, 2024

lwwmanning Oct 22, 2024

lwwmanning Oct 22, 2024

lwwmanning Oct 22, 2024

a10y Oct 22, 2024

lwwmanning Oct 22, 2024

a10y Oct 22, 2024

lwwmanning Oct 22, 2024

a10y Oct 22, 2024

a10y Oct 22, 2024

lwwmanning Oct 22, 2024

a10y Oct 22, 2024

a10y Oct 22, 2024

a10y Oct 22, 2024

lwwmanning Oct 22, 2024

lwwmanning Oct 22, 2024

a10y commented Oct 22, 2024

lwwmanning left a comment

		let array_arrow = self.clone().into_array().into_canonical()?.into_arrow()?;
		let array_ref = varbinview_as_arrow(self);

		}

		// TODO(aduffy): do we really need to do this with copying?

German strings, attempt 3 #1082

German strings, attempt 3 #1082

Conversation

a10y commented Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

lwwmanning commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert3005 commented Oct 18, 2024

robert3005 commented Oct 18, 2024

a10y commented Oct 18, 2024 • edited Loading

danking commented Oct 18, 2024

robert3005 commented Oct 18, 2024

a10y commented Oct 18, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

Vortex bytes_at

a10y commented Oct 22, 2024

a10y commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a10y commented Oct 22, 2024

lwwmanning left a comment

Choose a reason for hiding this comment

a10y commented Oct 17, 2024 •

edited

Loading

a10y commented Oct 18, 2024 •

edited

Loading