feat: Separate null count and minmax from column stats #11860

zation99 · 2024-12-14T02:40:08Z

Summary:
PR to address #11741

Removed the use of columnHasNulls in RowContainer and replaced them with row stats
Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated.

Differential Revision: D67229925

netlify · 2024-12-14T02:40:27Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`1b5122a`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67628605a4d98900082619cd

facebook-github-bot · 2024-12-14T02:40:29Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-14T02:44:51Z

This pull request was exported from Phabricator. Differential Revision: D67229925

xiaoxmeng

@zation99 thanks for the improvement % comments. Thanks!

velox/exec/HashProbe.cpp

xiaoxmeng · 2024-12-14T05:52:11Z

velox/exec/HashProbe.cpp

@@ -303,7 +303,7 @@ std::optional<uint64_t> HashProbe::estimatedRowSize(
  varSizeListColumnsStats.reserve(varSizedColumns.size());
  for (uint32_t i = 0; i < varSizedColumns.size(); ++i) {
    auto statsOpt = columnStats(varSizedColumns[i]);
-    if (!statsOpt.has_value()) {
+    if (!statsOpt.has_value() || !statsOpt->isMinMaxColumnStatsValid()) {


s/isMinMaxColumnStatsValid/minMaxColumnStatsValid/

xiaoxmeng · 2024-12-14T05:53:46Z

velox/exec/RowContainer.cpp

@@ -336,16 +334,38 @@ char* RowContainer::initializeRow(char* row, bool reuse) {
  return row;
 }

+/**


Put comment in header.

xiaoxmeng · 2024-12-14T06:43:29Z

velox/exec/RowContainer.h

+      minMaxStatsValid_ = false;
+    }
+
+    bool isMinMaxColumnStatsValid() const {


s/isMinMaxColumnStatsValid/minMaxColumnStatsValid/

xiaoxmeng · 2024-12-14T06:43:56Z

velox/exec/RowContainer.h

@@ -229,6 +250,7 @@ class RowColumn {

    uint32_t nonNullCount_{0};
    uint32_t nullCount_{0};
+    bool minMaxStatsValid_{true};


move this right after min max bytes?

xiaoxmeng · 2024-12-14T06:52:47Z

velox/exec/RowContainer.h

  inline bool columnHasNulls(int32_t columnIndex) const {
-    return columnHasNulls_[columnIndex];
+    return columnStats(columnIndex).has_value() &&


since we always has column stats now so can we change columnStats() to const RowColumn::Stats& RowContainer::columnStats? thanks!

It's unlikely but is it possible that it is initialized with empty rows?

The nullopt is to indicate invalidated column stats. Now the invalidation will be of finer granularity, So the column stats for each column will always be valid (not nullopt) but just some fields inside column stats invalidated under some conditions.

xiaoxmeng · 2024-12-14T06:54:31Z

velox/exec/RowContainer.h

  // fields. Index aligns with 'rowColumns_'.
  std::vector<RowColumn::Stats> rowColumnsStats_;
+  // Indicates if the min/max column stats are valid. This is set to false
+  // whenever a row is erased.
+  bool rowColumnsStatsMinMaxValid_ = true;


s/minMaxColumnStatsValid_/

We need to set it back to true in RowContainer::clear() and please also comment about this

xiaoxmeng · 2024-12-14T06:56:05Z

velox/exec/RowContainer.cpp

 void RowContainer::eraseRows(folly::Range<char**> rows) {
  freeRowsExtraMemory(rows, /*freeNextRowVector=*/true);
  for (auto* row : rows) {
    VELOX_CHECK(!bits::isBitSet(row, freeFlagOffset_), "Double free of row");
    bits::setBit(row, freeFlagOffset_);
    nextFree(row) = firstFreeRow_;
    firstFreeRow_ = row;
+
+    removeRowFromColumnStats(row);


s/removeRowFromColumnStats/removeRowColumnStats/

Hmm... it is actually meant to remove some rows info from the rowColumnStats. removeRowColumnStats sounds like we removed all the stats. wdyt?

xiaoxmeng · 2024-12-14T06:57:27Z

velox/exec/RowContainer.h

-  // columnHasNulls_ flag is false by default.
-  inline void updateColumnHasNulls(int32_t columnIndex, bool hasNulls) {
-    columnHasNulls_[columnIndex] = columnHasNulls_[columnIndex] || hasNulls;
+  void invalidateMinMaxColumnStats() {


When we use this?

When we have eraseRows or setAllNull

Maybe some additional ask. And maybe we can discuss this and potentially have a followup: Fixed column size stats can immune from erasure. Do we want to indicate in the column stats if this column is fixed/variable length column? We just need to set the stats size at construction time and skip updating them. Might save some cycles.

Good idea. I'll make a follow up task for this.

velox/exec/tests/RowContainerTest.cpp

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-14T09:40:15Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-14T23:50:02Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-15T04:47:32Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-15T04:51:48Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-15T04:56:08Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-15T21:08:56Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-15T21:24:23Z

This pull request was exported from Phabricator. Differential Revision: D67229925

xiaoxmeng · 2024-12-16T18:57:08Z

velox/exec/RowContainer.h

@@ -316,15 +336,28 @@ class RowContainer {
             : 0);
  }

+  /**


velox/exec/tests/RowContainerTest.cpp

velox/exec/RowContainer.h

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-17T00:18:45Z

This pull request was exported from Phabricator. Differential Revision: D67229925

tanjialiang · 2024-12-17T07:01:11Z

velox/exec/RowContainer.h

    /// Merges multiple aggregated stats of the same column into a single one.
    static Stats merge(const std::vector<Stats>& statsList);

   private:
    // Aggregated stats for non-null rows of the column.
    int32_t minBytes_{0};
    int32_t maxBytes_{0};
+    bool minMaxStatsValid_{true};


Can we make the name of this variable more general? We could potentially have some more stats that could be invalidable.
And shall we add comments to the part of the stats that are invalidable and the other part of the stats that are not? Something like:

int32_t minBytes_{0};
int32_t maxBytes_{0};

// Flag that ... above stats ...
bool fragileStatsValid_{true};

uint64_t sumBytes_{0};
uint32_t nonNullCount_{0};
uint32_t nullCount_{0};

I tend to leave minMax name here for now to make it more explicit so people easily know what stats are invalidated. But in the future, if there are more stats that cannot be tracked, yes I think we should rename it.

velox/exec/RowContainer.h

tanjialiang · 2024-12-17T07:35:40Z

velox/exec/RowContainer.h

+  /// @param keepNullCount - If true, the null count is kept if originally null
+  /// or increase if originally not null.


If true, the stats is updated as if the row is replaced with an all-null row.

velox/exec/RowContainer.h

tanjialiang · 2024-12-17T07:53:46Z

Thanks @zation99 for the changes! Left some comments and we can have some discussions offline.

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-18T00:19:23Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-18T06:47:28Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-18T07:34:07Z

This pull request was exported from Phabricator. Differential Revision: D67229925

…ator#11860) Summary: PR to address facebookincubator#11741 - Removed the use of columnHasNulls in RowContainer and replaced them with row stats - Separate null count/sumBytes from minmax. In the case of rows erasure, only min/max is invalidated. Differential Revision: D67229925

facebook-github-bot · 2024-12-18T08:21:51Z

This pull request was exported from Phabricator. Differential Revision: D67229925

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 Dec 14, 2024

facebook-github-bot added the fb-exported label Dec 14, 2024

zation99 requested review from xiaoxmeng and tanjialiang December 14, 2024 02:40

zation99 force-pushed the export-D67229925 branch from c6c4c8c to 329f9a9 Compare December 14, 2024 02:44

xiaoxmeng reviewed Dec 14, 2024

View reviewed changes

zation99 force-pushed the export-D67229925 branch from 329f9a9 to 8f9663b Compare December 14, 2024 09:39

zation99 force-pushed the export-D67229925 branch from 8f9663b to 592bcf9 Compare December 14, 2024 23:49

zation99 force-pushed the export-D67229925 branch from 592bcf9 to 4f90106 Compare December 15, 2024 04:47

zation99 force-pushed the export-D67229925 branch from 4f90106 to cb4e8af Compare December 15, 2024 04:51

zation99 force-pushed the export-D67229925 branch from cb4e8af to 605a4e2 Compare December 15, 2024 04:55

zation99 force-pushed the export-D67229925 branch from 605a4e2 to 067ec67 Compare December 15, 2024 21:08

zation99 force-pushed the export-D67229925 branch from 067ec67 to 8425802 Compare December 15, 2024 21:23

zation99 requested a review from xiaoxmeng December 16, 2024 05:42

xiaoxmeng reviewed Dec 16, 2024

View reviewed changes

velox/exec/RowContainer.h Outdated Show resolved Hide resolved

zation99 force-pushed the export-D67229925 branch from 8425802 to 108558f Compare December 17, 2024 00:18

tanjialiang reviewed Dec 17, 2024

View reviewed changes

velox/exec/RowContainer.h Outdated Show resolved Hide resolved

tanjialiang reviewed Dec 17, 2024

View reviewed changes

velox/exec/RowContainer.h Outdated Show resolved Hide resolved

tanjialiang reviewed Dec 17, 2024

View reviewed changes

velox/exec/RowContainer.h Outdated Show resolved Hide resolved

zation99 force-pushed the export-D67229925 branch from 108558f to c1e7abe Compare December 18, 2024 00:18

zation99 force-pushed the export-D67229925 branch from c1e7abe to 94828f7 Compare December 18, 2024 06:46

zation99 force-pushed the export-D67229925 branch from 94828f7 to c7d3840 Compare December 18, 2024 07:33

zation99 force-pushed the export-D67229925 branch from c7d3840 to 1b5122a Compare December 18, 2024 08:21

@@ @@ -316,15 +336,28 @@ class RowContainer { @@
                            : 0);
                 }
+                /**

		/// @param keepNullCount - If true, the null count is kept if originally null
		/// or increase if originally not null.

feat: Separate null count and minmax from column stats #11860

Are you sure you want to change the base?

feat: Separate null count and minmax from column stats #11860

Conversation

zation99 commented Dec 14, 2024

netlify bot commented Dec 14, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Dec 14, 2024

facebook-github-bot commented Dec 14, 2024

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

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

facebook-github-bot commented Dec 14, 2024

facebook-github-bot commented Dec 14, 2024

facebook-github-bot commented Dec 15, 2024

facebook-github-bot commented Dec 15, 2024

facebook-github-bot commented Dec 15, 2024

facebook-github-bot commented Dec 15, 2024

facebook-github-bot commented Dec 15, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Dec 17, 2024

tanjialiang Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanjialiang commented Dec 17, 2024

facebook-github-bot commented Dec 18, 2024

facebook-github-bot commented Dec 18, 2024

facebook-github-bot commented Dec 18, 2024

facebook-github-bot commented Dec 18, 2024

netlify bot commented Dec 14, 2024 •

edited

Loading

tanjialiang Dec 17, 2024 •

edited

Loading