GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading only one value #15124

mapleFU · 2022-12-30T00:19:57Z

This patch trying to fix #15052 . The problem is mentioned here: #15052 (comment)

When read 1 value, DeltaBitPackDecoder will not call InitBlock, causing it always read last_value_.

Seems the problem is introduced in #10627 and amol-@d982bed

I will add some test tonight

Closes: [C++][Parquet] Getting only 0 when reading DELTA_BINARY_PACKED #15052

github-actions · 2022-12-30T00:20:18Z

Closes: [C++][Parquet] Getting only 0 when reading DELTA_BINARY_PACKED #15052

github-actions · 2022-12-30T00:20:21Z

⚠️ GitHub issue #15052 has been automatically assigned in GitHub to PR creator.

github-actions · 2022-12-30T00:20:22Z

⚠️ GitHub issue #15052 has no components, please add labels for components.

github-actions · 2022-12-30T12:53:06Z

⚠️ GitHub issue #15052 has no components, please add labels for components.

rok

Thanks for doing this @mapleFU! This is a valuable fix.

rok · 2022-12-30T14:32:46Z

cpp/src/parquet/encoding.cc

          buffer[i++] = last_value_;
-          if (ARROW_PREDICT_FALSE(i == max_values)) break;
+          if (ARROW_PREDICT_FALSE(i == max_values)) {
+            if (i != static_cast<int>(total_value_count_)) {
+              InitBlock();
+            }
+            break;
+          }


I find this difficult to read, could you add a comment about what this logic is doing?

To be honest, the logic is a bit trickey here, I will try to explain it. I'm bad in English, so maybe you can edit it directly if you can explain it better.

cpp/src/parquet/encoding.cc

cpp/src/parquet/encoding_test.cc

mapleFU · 2022-12-30T16:49:43Z

@tachyonwill I think this patch might fix DELTA_BINARY_PACKED issue, mind take a look?

cpp/src/parquet/encoding.cc

Change the comments for fixing Co-authored-by: Rok Mihevc <rok@mihevc.org>

mapleFU · 2022-12-31T12:21:34Z

Retrigger CI because I think the fail is not caused by me, but still it looks like a bug: https://github.com/apache/arrow/actions/runs/3808810467/jobs/6479699758

By the way, is there any method that I can retrigger the CI?

… into parquet/fix-gh-15052

mapleFU · 2022-12-31T13:45:52Z

@pitrou Mind take a look here?

pitrou · 2022-12-31T13:46:28Z

@pitrou Mind take a look here?

In a few days certainly :-)

rok · 2023-01-02T13:30:28Z

By the way, is there any method that I can retrigger the CI?

Once your first PR is merged CI will start automatically.

cpp/src/parquet/encoding.cc

cpp/src/parquet/encoding_test.cc

mapleFU · 2023-01-03T13:52:08Z

oops, I got a

=================================== FAILURES ===================================
______________________ [doctest] pyarrow._fs.PyFileSystem ______________________
EXAMPLE LOCATION UNKNOWN, not showing all tests of that example
??? >>> gfs = github.GithubFileSystem('apache', 'arrow', sha='ec51aec4d15035f4d9d6a1c4346d0a2b9a37fb75')
UNEXPECTED EXCEPTION: HTTPError('403 Client Error: rate limit exceeded for url: https://api.github.com/repos/apache/arrow/git/trees/ec51aec4d15035f4d9d6a1c4346d0a2b9a37fb75')
Traceback (most recent call last):
  File "/opt/conda/envs/arrow/lib/python3.9/doctest.py", line 1334, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest pyarrow._fs.PyFileSystem[1]>", line 1, in <module>
  File "/opt/conda/envs/arrow/lib/python3.9/site-packages/fsspec/spec.py", line 76, in __call__
    obj = super().__call__(*args, **kwargs)
  File "/opt/conda/envs/arrow/lib/python3.9/site-packages/fsspec/implementations/github.py", line 56, in __init__
    self.ls("")
  File "/opt/conda/envs/arrow/lib/python3.9/site-packages/fsspec/implementations/github.py", line 158, in ls
    r.raise_for_status()
  File "/opt/conda/envs/arrow/lib/python3.9/site-packages/requests/models.py", line 1021, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 403 Client Error: rate limit exceeded for url: https://api.github.com/repos/apache/arrow/git/trees/ec51aec4d15035f4d9d6a1c4346d0a2b9a37fb75
/opt/conda/envs/arrow/lib/python3.9/site-packages/pyarrow/_fs.cpython-39-x86_64-linux-gnu.so:None: UnexpectedException
=============================== warnings summary ===============================
opt/conda/envs/arrow/lib/python3.9/site-packages/pytest_cython/plugin.py:57
  /opt/conda/envs/arrow/lib/python3.9/site-packages/pytest_cython/plugin.py:57: PytestRemovedIn8Warning: The (fspath: py.path.local) argument to DoctestModule is deprecated. Please use the (path: pathlib.Path) argument instead.
  See https://docs.pytest.org/en/latest/deprecations.html#fspath-argument-for-node-constructors-replaced-with-pathlib-path
    return DoctestModule.from_parent(parent, fspath=path)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
SKIPPED [2] opt/conda/envs/arrow/lib/python3.9/site-packages/_pytest/doctest.py:455: all tests skipped by +SKIP option
============= 1 failed, 223 passed, 2 skipped, 1 warning in 8.70s ==============

Seems that maybe we should rerun it?

pitrou · 2023-01-03T14:01:01Z

oops, I got a

[...]

Seems that maybe we should rerun it?

No worries, this is unrelated to his PR.

mapleFU · 2023-01-03T14:18:48Z

Well, it would actually be simpler without adding a separate test method... and it would exercise the code more thoroughly as well.

So how can I organize that code? I didn't make it clear. Is it like:

case 1

template <typename Type>
class TestEncodingBase : public ::testing::Test {
 public:
...
 private:
   std::vector<int> small_read_batch_size_;
};

and

TYPED_TEST(TestDeltaBitPackEncoding, SmallBatchRoundTrip) {
  // .. set draws_ 1, 3
  ASSERT_NO_FATAL_FAILURE(this->Execute(1, 1));
  ASSERT_NO_FATAL_FAILURE(this->Execute(1, 2));
  ASSERT_NO_FATAL_FAILURE(this->Execute(2, 2));
  ASSERT_NO_FATAL_FAILURE(this->ExecuteSteps(10, 10));
  ASSERT_NO_FATAL_FAILURE(this->ExecuteSteps(10, 10));
  ASSERT_NO_FATAL_FAILURE(this->ExecuteSteps(1, 10));
}

Or

case 2

template <typename Type>
class TestDeltaBitPackEncoding : public TestEncodingBase<Type> {
 public:
  using c_type = typename Type::c_type;
  static constexpr int TYPE = Type::type_num;
  static constexpr size_t ROUND_TRIP_TIMES = 3;

  void InitReadBatchSizedData(...);
};

Can you help me make it clear? @pitrou

pitrou · 2023-01-03T14:48:34Z

You could have something like:

  const std::vector<int> kBatchSizes = {...};

  void CheckRoundtrip() override {
    auto encoder =
        MakeTypedEncoder<Type>(Encoding::DELTA_BINARY_PACKED, false, descr_.get());
    auto decoder = MakeTypedDecoder<Type>(Encoding::DELTA_BINARY_PACKED, descr_.get());
    for (size_t i = 0; i < ROUND_TRIP_TIMES; ++i) {
      for (const auto batch_size : kBatchSizes} {
        ...

mapleFU · 2023-01-03T15:30:55Z

Seems that would causing our testing run a long time, should I make kBatchSizes configurable, and just using another Execute? Like:

template <typename Type>
class TestDeltaBitPackEncoding : public TestEncodingBase<Type> {
 public:
  using c_type = typename Type::c_type;
  static constexpr int TYPE = Type::type_num;
  static constexpr size_t ROUND_TRIP_TIMES = 3;

  void InitReadBatchSizedData(...);
};

  void CheckRoundtrip() override {
    ...
    auto read_batch_sizes = extra_read_batch_sizes;
    read_batch_sizes.push_back(num_values_);
    for (size_t i = 0; i < ROUND_TRIP_TIMES; ++i) {
      for (int read_batch_size: read_batch_sizes) {
        encoder->Put(draws_, num_values_);
        encode_buffer_ = encoder->FlushValues();

        decoder->SetData(num_values_, encode_buffer_->data(),
                         static_cast<int>(encode_buffer_->size()));

        int values_decoded_sum = 0;
        while (values_decoded_sum < num_values_) {
          int values_decoded =
              decoder->Decode(decode_buf_ + values_decoded_sum, read_batch_size);
          values_decoded_sum += values_decoded;
        }
        ASSERT_EQ(num_values_, values_decoded_sum);
        ASSERT_NO_FATAL_FAILURE(VerifyResults<c_type>(decode_buf_, draws_, num_values_));
      }
    }
  }

and

void ExecuteSteps(int nvalues, int repeats, std::vector<int> extra_read_batch_size) {
  this->InitReadBatchSizedData(nvalues, repeats, extra_read_batch_size);
  this->CheckRoundtrip();
}

I'm arguing this, because when I set const std::vector<int> EXTRA_READ_BATCH_SIZES = {1, 10}; and make CheckRoundTrip run it every time, the testing will spend 5 minutes in debug mode, which makes me mad :( ExecuteSteps will only finished in one second

mapleFU · 2023-01-04T01:55:42Z

@pitrou comment fixed and merged your patch which could make test faster. Mind take a look?

pitrou

+1

ursabot · 2023-01-05T01:02:16Z

Benchmark runs are scheduled for baseline = ec9a8a3 and contender = 53d73f8. 53d73f8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.11% ⬆️0.0%] test-mac-arm
[Finished ⬇️12.24% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️2.86% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 53d73f8f ec2-t3-xlarge-us-east-2
[Failed] 53d73f8f test-mac-arm
[Finished] 53d73f8f ursa-i9-9960x
[Failed] 53d73f8f ursa-thinkcentre-m75q
[Finished] ec9a8a32 ec2-t3-xlarge-us-east-2
[Failed] ec9a8a32 test-mac-arm
[Finished] ec9a8a32 ursa-i9-9960x
[Failed] ec9a8a32 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-01-05T01:02:33Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

…eading only one value (apache#15124) This patch trying to fix apache#15052 . The problem is mentioned here: apache#15052 (comment) When read 1 value, DeltaBitPackDecoder will not call `InitBlock`, causing it always read `last_value_`. Seems the problem is introduced in apache#10627 and amol-@d982bed I will add some test tonight * Closes: apache#15052 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: mwish <1506118561@qq.com> Co-authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

Move InitBlock ahead

fe6ceea

mapleFU mentioned this pull request Dec 30, 2022

[C++][Parquet] Getting only 0 when reading DELTA_BINARY_PACKED #15052

Closed

github-actions bot added Component: C++ Component: Parquet labels Dec 30, 2022

mapleFU added 2 commits December 30, 2022 21:45

[Update] Handling the case for only one value

2adb755

[ADD] add some test with different steps

f978e25

rok approved these changes Dec 30, 2022

View reviewed changes

rok reviewed Dec 30, 2022

View reviewed changes

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

address comment

0abb863

mapleFU force-pushed the parquet/fix-gh-15052 branch from 31f7558 to 0abb863 Compare December 30, 2022 14:58

mapleFU requested a review from rok December 30, 2022 15:02

rok reviewed Dec 30, 2022

View reviewed changes

cpp/src/parquet/encoding.cc Outdated Show resolved Hide resolved

Update commit in cpp/src/parquet/encoding.cc

3c0a611

Change the comments for fixing Co-authored-by: Rok Mihevc <rok@mihevc.org>

mapleFU added 2 commits December 31, 2022 20:22

Merge branch 'master' into parquet/fix-gh-15052

2461292

Merge branch 'parquet/fix-gh-15052' of https://github.com/mapleFU/arrow…

a019574

… into parquet/fix-gh-15052

mapleFU force-pushed the parquet/fix-gh-15052 branch from 642d779 to a019574 Compare December 31, 2022 13:45

pitrou reviewed Jan 3, 2023

View reviewed changes

cpp/src/parquet/encoding.cc Outdated Show resolved Hide resolved

pitrou reviewed Jan 3, 2023

View reviewed changes

cpp/src/parquet/encoding.cc Show resolved Hide resolved

pitrou reviewed Jan 3, 2023

View reviewed changes

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

mapleFU added 2 commits January 3, 2023 19:43

Merge branch 'master' into parquet/fix-gh-15052

c85651c

address comment

7125483

mapleFU requested a review from pitrou January 3, 2023 11:55

mapleFU added 2 commits January 3, 2023 23:46

add extra_batch_size, and remove ExecuteSteps

1cf2511

Merge branch 'master' into parquet/fix-gh-15052

18c5964

Nits

be8436c

pitrou approved these changes Jan 4, 2023

View reviewed changes

pitrou changed the title ~~GH-15052: [C++][Parquet] Fixing DELTA_BINARY_PACKED when reading only 1~~ GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading only one value Jan 4, 2023

pitrou merged commit 53d73f8 into apache:master Jan 4, 2023

mapleFU deleted the parquet/fix-gh-15052 branch January 4, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading only one value #15124

GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading only one value #15124

mapleFU commented Dec 30, 2022 •

edited

github-actions bot commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

rok left a comment

rok Dec 30, 2022

mapleFU Dec 30, 2022

mapleFU commented Dec 30, 2022

mapleFU commented Dec 31, 2022

mapleFU commented Dec 31, 2022

pitrou commented Dec 31, 2022

rok commented Jan 2, 2023

mapleFU commented Jan 3, 2023

pitrou commented Jan 3, 2023

mapleFU commented Jan 3, 2023

pitrou commented Jan 3, 2023

mapleFU commented Jan 3, 2023

mapleFU commented Jan 4, 2023

pitrou left a comment

ursabot commented Jan 5, 2023

ursabot commented Jan 5, 2023

GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading only one value #15124

GH-15052: [C++][Parquet] Fix DELTA_BINARY_PACKED decoder when reading only one value #15124

Conversation

mapleFU commented Dec 30, 2022 • edited

github-actions bot commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

github-actions bot commented Dec 30, 2022

rok left a comment

Choose a reason for hiding this comment

rok Dec 30, 2022

Choose a reason for hiding this comment

mapleFU Dec 30, 2022

Choose a reason for hiding this comment

mapleFU commented Dec 30, 2022

mapleFU commented Dec 31, 2022

mapleFU commented Dec 31, 2022

pitrou commented Dec 31, 2022

rok commented Jan 2, 2023

mapleFU commented Jan 3, 2023

pitrou commented Jan 3, 2023

mapleFU commented Jan 3, 2023

case 1

case 2

pitrou commented Jan 3, 2023

mapleFU commented Jan 3, 2023

mapleFU commented Jan 4, 2023

pitrou left a comment

Choose a reason for hiding this comment

ursabot commented Jan 5, 2023

ursabot commented Jan 5, 2023

mapleFU commented Dec 30, 2022 •

edited