SplitReader refactor #8995

yingsu00 · 2024-03-07T06:19:30Z

To prepare for the upcoming equality delete file read, we need to refactor the SplitReader a bit.

netlify · 2024-03-07T06:19:46Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`db8e894`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/660bfb7d954a0e0007f965c3

velox/connectors/hive/SplitReader.h

Yuhta · 2024-03-12T15:56:03Z

velox/connectors/hive/SplitReader.h

@@ -93,7 +94,7 @@ class SplitReader {
      std::shared_ptr<common::MetadataFilter> metadataFilter,
      dwio::common::RuntimeStatistics& runtimeStats);

-  virtual uint64_t next(int64_t size, VectorPtr& output);
+  virtual uint64_t next(uint64_t size, VectorPtr& output);


Leave it as signed for safer comparison

Leave it as signed for safer comparison

@Yuhta The HiveDataSource::next(uint64_t size, velox::ContinueFuture& /*future*/) and Reader::next(next(uint64_t size,velox::VectorPtr& result,..) both use uint64_t, and size was directly passed to downstream functions and never gets changed. The the uint64_t -> int64_t -> uint64_t change may cause problem instead. Shall we make all the 3 function signatures align?

In fact, I should have been using uint64_t in SplitReader::next() when creating it, but somehow don't remember why making it int64_t. Now I realized this is not desired and thus this fix.

OK let's use uint64_t to keep it consistent with data source and file readers in this case then. In general for new interfaces we should prefer signed types for any quantity variable, because it's much safer with the combination of subtraction and comparison

velox/connectors/hive/iceberg/IcebergSplitReader.cpp

velox/connectors/hive/iceberg/IcebergSplitReader.h

velox/connectors/hive/SplitReader.cpp

yingsu00 · 2024-03-18T08:33:19Z

@Yuhta I have addressed all comments except the int64_t -> uint64_t one, which is waiting for your reply. Can you please review again? Thanks!

facebook-github-bot · 2024-03-19T15:27:08Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@yingsu00 thanks for the refactor % some minors.

xiaoxmeng · 2024-03-19T16:20:57Z

velox/connectors/hive/SplitReader.cpp

@@ -141,6 +141,81 @@ void SplitReader::configureReaderOptions(
 void SplitReader::prepareSplit(
    std::shared_ptr<common::MetadataFilter> metadataFilter,
    dwio::common::RuntimeStatistics& runtimeStats) {
+  createReader();
+
+  if (testEmptySplit(runtimeStats)) {


Is it good or reliable to do empty check based on runtime stats?

s/testEmptySplit/checkIfEmptySplit/

@xiaoxmeng The check is not BASED on runtimeStats, but it needs to update it if the split can be skipped.
Regarding the name, would isEmptySplit or hasEmptySplit or splitIsEmpty or isSplitEmpty be better? These names won't cause confusion whether returning true is empty or not empty.

velox/connectors/hive/SplitReader.h

xiaoxmeng · 2024-03-19T16:29:29Z

velox/connectors/hive/SplitReader.cpp


-  // Note that this doesn't apply to Hudi tables.
+bool SplitReader::testEmptySplit(
+    dwio::common::RuntimeStatistics& runtimeStats) {
  emptySplit_ = false;


createReader might have set emptySplit_ to true and do we want to revert? And do we support to reuse the same SplitReader on the same split? If not, let's have more sanity check here? Thanks!

@xiaoxmeng Thanks for catching this. One SplitReader object is created for each split, and prepareSplit() is supposed to be called once only. I have made the following change here:

bool SplitReader::splitIsEmpty(dwio::common::RuntimeStatistics& runtimeStats) { // emptySplit_ may already be set if the data file is not found. In this case // we don't need to test further. if (emptySplit_) { return true; } ...

velox/connectors/hive/SplitReader.cpp

xiaoxmeng · 2024-03-19T16:44:46Z

velox/connectors/hive/SplitReader.cpp

+    return DataSource::kUnknownRowSize;
+  }
+
+  auto size = baseRowReader_->estimatedRowSize();


nit:

const auto sizeOr = baseRowReader_->estimatedRowSize(); return sizeOr.valueOr(DataSource::kUnknownRowSize);

Would you update this? Thanks!

xiaoxmeng · 2024-03-19T16:47:05Z

velox/connectors/hive/iceberg/IcebergSplitReader.h

+      const std::shared_ptr<velox::connector::hive::HiveConnectorSplit>&
+          hiveSplit,
+      const std::shared_ptr<HiveTableHandle>& hiveTableHandle,
+      const std::shared_ptr<common::ScanSpec>& scanSpec,
      const RowTypePtr readerOutputType,


nit: const RowTypePtr&

xiaoxmeng

@yingsu00 LGTM % nits. Thanks for the update!

xiaoxmeng · 2024-03-24T05:55:01Z

velox/connectors/hive/iceberg/IcebergSplitReader.h

+      const std::shared_ptr<const HiveConfig>& hiveConfig,
+      const RowTypePtr& readerOutputType,
+      const std::shared_ptr<io::IoStatistics>& ioStats,
+      FileHandleFactory* const fileHandleFactory,


s/FileHandleFactory* const fileHandleFactory/FileHandleFactory* fileHandleFactory/

xiaoxmeng · 2024-03-24T05:56:10Z

velox/connectors/hive/SplitReader.h

-          partitionKeys,
-      FileHandleFactory* fileHandleFactory,
+      const std::shared_ptr<io::IoStatistics>& ioStats,
+      FileHandleFactory* const fileHandleFactory,


xiaoxmeng · 2024-03-24T05:56:15Z

velox/connectors/hive/SplitReader.h

-          partitionKeys,
-      FileHandleFactory* fileHandleFactory,
+      const std::shared_ptr<io::IoStatistics>& ioStats,
+      FileHandleFactory* const fileHandleFactory,


xiaoxmeng · 2024-03-24T05:58:49Z

velox/connectors/hive/SplitReader.cpp

+    return DataSource::kUnknownRowSize;
+  }
+
+  auto size = baseRowReader_->estimatedRowSize();


Would you update this? Thanks!

xiaoxmeng · 2024-03-24T05:59:58Z

velox/connectors/hive/SplitReader.cpp

-  if (baseReader_->numberOfRows() == 0) {
-    emptySplit_ = true;
-    return;
+bool SplitReader::splitIsEmpty(dwio::common::RuntimeStatistics& runtimeStats) {


Shall we call this checkIfSplitEmpty as we update emptySplit_ in this function and SplitReader already has emptySplit() method which returns emptySplit_. Thanks!

Ok, renamed to checkIfSplitIsEmpty()

xiaoxmeng · 2024-03-24T06:00:32Z

velox/connectors/hive/SplitReader.cpp

+  createReader();
+
+  if (splitIsEmpty(runtimeStats)) {
+    return;


nit: VELOX_CHECK(emptySplit_);

xiaoxmeng · 2024-03-24T06:01:06Z

velox/connectors/hive/iceberg/IcebergSplitReader.cpp

+    const std::shared_ptr<const HiveConfig>& hiveConfig,
+    const RowTypePtr& readerOutputType,
+    const std::shared_ptr<io::IoStatistics>& ioStats,
+    FileHandleFactory* const fileHandleFactory,


xiaoxmeng · 2024-03-24T06:01:13Z

velox/connectors/hive/iceberg/IcebergSplitReader.cpp

+  createReader();
+
+  if (splitIsEmpty(runtimeStats)) {
+    return;


To prepare for the upcoming equality delete file read, we need to refactor the SplitReader and break prepareSplit() into several parts. This commit does this, and also include a couple of other code cleanups.

This commit reorders the function implementations in SplitReader.cpp to make it align with the order of their declarations in SplitReader.h, which is organized with their access levels and call orders.

Reorder SplitReader class fields

yingsu00 · 2024-04-02T14:49:28Z

@Yuhta @xiaoxmeng All comments addressed, will you please reimport? Many thanks!

facebook-github-bot · 2024-04-10T19:49:21Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-04-10T22:58:30Z

@Yuhta merged this pull request in b5ea2d7.

conbench-facebook · 2024-04-10T23:19:40Z

Conbench analyzed the 1 benchmark run on commit b5ea2d72.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: To prepare for the upcoming equality delete file read, we need to refactor the SplitReader a bit. Pull Request resolved: facebookincubator#8995 Reviewed By: xiaoxmeng Differential Revision: D55072998 Pulled By: Yuhta fbshipit-source-id: 662459041b947d51ffaa98b57a50e4ebdd5b36e3

This reverts commit b5ea2d7.

Summary: To prepare for the upcoming equality delete file read, we need to refactor the SplitReader a bit. Pull Request resolved: facebookincubator#8995 Reviewed By: xiaoxmeng Differential Revision: D55072998 Pulled By: Yuhta fbshipit-source-id: 662459041b947d51ffaa98b57a50e4ebdd5b36e3

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2024

yingsu00 force-pushed the refactor branch 6 times, most recently from 67dc076 to 227571f Compare March 12, 2024 10:42

yingsu00 marked this pull request as ready for review March 12, 2024 15:38

yingsu00 requested a review from Yuhta March 12, 2024 15:39

mbasmanova requested a review from xiaoxmeng March 13, 2024 07:16

Yuhta reviewed Mar 13, 2024

View reviewed changes

yingsu00 force-pushed the refactor branch from 227571f to 9fb0c58 Compare March 16, 2024 14:30

xiaoxmeng reviewed Mar 19, 2024

View reviewed changes

yingsu00 force-pushed the refactor branch 2 times, most recently from e0040c1 to cbbfa10 Compare March 21, 2024 13:41

xiaoxmeng approved these changes Mar 24, 2024

View reviewed changes

yingsu00 force-pushed the refactor branch 5 times, most recently from 1f88646 to 411c788 Compare April 2, 2024 10:10

yingsu00 added 4 commits April 2, 2024 20:34

SplitReader refactor

757d853

To prepare for the upcoming equality delete file read, we need to refactor the SplitReader and break prepareSplit() into several parts. This commit does this, and also include a couple of other code cleanups.

Reorder the functions in SplitReader.cpp

e02c0a4

This commit reorders the function implementations in SplitReader.cpp to make it align with the order of their declarations in SplitReader.h, which is organized with their access levels and call orders.

Enforce const-ness for SplitReader when necessary

5dbe47b

Reorder SplitReader fields

db8e894

Reorder SplitReader class fields

yingsu00 force-pushed the refactor branch from 411c788 to db8e894 Compare April 2, 2024 12:35

facebook-github-bot closed this in b5ea2d7 Apr 10, 2024

facebook-github-bot added the Merged label Apr 10, 2024

liujiayi771 mentioned this pull request Apr 11, 2024

Remove partitionKey and partitionKeysHandle size check in testFilters #9450

Closed

marin-ma added a commit to marin-ma/velox-oap that referenced this pull request Apr 15, 2024

Revert "SplitReader refactor (facebookincubator#8995)"

1492034

This reverts commit b5ea2d7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SplitReader refactor #8995

SplitReader refactor #8995

yingsu00 commented Mar 7, 2024

netlify bot commented Mar 7, 2024 •

edited

Loading

Yuhta Mar 12, 2024

yingsu00 Mar 16, 2024 •

edited

Loading

Yuhta Mar 19, 2024

yingsu00 commented Mar 18, 2024

facebook-github-bot commented Mar 19, 2024

xiaoxmeng left a comment

xiaoxmeng Mar 19, 2024

yingsu00 Mar 20, 2024 •

edited

Loading

xiaoxmeng Mar 19, 2024

yingsu00 Mar 20, 2024

xiaoxmeng Mar 19, 2024

xiaoxmeng Mar 24, 2024

yingsu00 Mar 29, 2024

xiaoxmeng Mar 19, 2024

xiaoxmeng left a comment

xiaoxmeng Mar 24, 2024

xiaoxmeng Mar 24, 2024

yingsu00 Mar 29, 2024

xiaoxmeng Mar 24, 2024

yingsu00 Mar 29, 2024

xiaoxmeng Mar 24, 2024

xiaoxmeng Mar 24, 2024 •

edited

Loading

yingsu00 Mar 29, 2024

xiaoxmeng Mar 24, 2024

yingsu00 Mar 29, 2024

xiaoxmeng Mar 24, 2024

yingsu00 Mar 29, 2024

xiaoxmeng Mar 24, 2024

yingsu00 Apr 2, 2024

yingsu00 commented Apr 2, 2024

facebook-github-bot commented Apr 10, 2024

facebook-github-bot commented Apr 10, 2024

conbench-facebook bot commented Apr 10, 2024

SplitReader refactor #8995

SplitReader refactor #8995

Conversation

yingsu00 commented Mar 7, 2024

netlify bot commented Mar 7, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Choose a reason for hiding this comment

yingsu00 Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yingsu00 commented Mar 18, 2024

facebook-github-bot commented Mar 19, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yingsu00 Mar 20, 2024 • edited Loading

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

xiaoxmeng left a 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

xiaoxmeng Mar 24, 2024 • edited Loading

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

yingsu00 commented Apr 2, 2024

facebook-github-bot commented Apr 10, 2024

facebook-github-bot commented Apr 10, 2024

conbench-facebook bot commented Apr 10, 2024

netlify bot commented Mar 7, 2024 •

edited

Loading

yingsu00 Mar 16, 2024 •

edited

Loading

yingsu00 Mar 20, 2024 •

edited

Loading

xiaoxmeng Mar 24, 2024 •

edited

Loading