From 2c57445fd38dbf41794c12ed5a221cc02c49cdb9 Mon Sep 17 00:00:00 2001 From: zuyu Date: Tue, 3 Dec 2024 18:53:42 -0800 Subject: [PATCH] refactor: Use const ref in Filter (#11725) Summary: For `int128_t` and `Timestamp` types. Pull Request resolved: https://github.com/facebookincubator/velox/pull/11725 Reviewed By: Yuhta Differential Revision: D66726951 Pulled By: bikramSingh91 fbshipit-source-id: dff4ea944790f6076764eaafb5e3fdafd67548ef --- .../parquet/reader/TimestampColumnReader.h | 4 +- velox/type/Filter.cpp | 8 +- velox/type/Filter.h | 113 ++++++++++-------- 3 files changed, 70 insertions(+), 55 deletions(-) diff --git a/velox/dwio/parquet/reader/TimestampColumnReader.h b/velox/dwio/parquet/reader/TimestampColumnReader.h index 308743ecea4f..5864cc31f1cb 100644 --- a/velox/dwio/parquet/reader/TimestampColumnReader.h +++ b/velox/dwio/parquet/reader/TimestampColumnReader.h @@ -23,7 +23,7 @@ namespace facebook::velox::parquet { namespace { // Range filter for Parquet Int96 Timestamp. -class ParquetInt96TimestampRange : public common::TimestampRange { +class ParquetInt96TimestampRange final : public common::TimestampRange { public: // @param lower Lower end of the range, inclusive. // @param upper Upper end of the range, inclusive. @@ -36,7 +36,7 @@ class ParquetInt96TimestampRange : public common::TimestampRange { // Int96 is read as int128_t value and converted to Timestamp by extracting // days and nanos. - bool testInt128(int128_t value) const final override { + bool testInt128(const int128_t& value) const final { const int32_t days = static_cast(value >> 64); const uint64_t nanos = value & ((((1ULL << 63) - 1ULL) << 1) + 1); const auto ts = Timestamp::fromDaysAndNanos(days, nanos); diff --git a/velox/type/Filter.cpp b/velox/type/Filter.cpp index d0f6932b4aad..f00090f4513b 100644 --- a/velox/type/Filter.cpp +++ b/velox/type/Filter.cpp @@ -991,8 +991,8 @@ FilterPtr HugeintValuesUsingHashTable::create(const folly::dynamic& obj) { } HugeintValuesUsingHashTable::HugeintValuesUsingHashTable( - const int128_t min, - const int128_t max, + const int128_t& min, + const int128_t& max, const std::vector& values, const bool nullAllowed) : Filter(true, nullAllowed, FilterKind::kHugeintValuesUsingHashTable), @@ -1005,7 +1005,7 @@ HugeintValuesUsingHashTable::HugeintValuesUsingHashTable( } } -bool HugeintValuesUsingHashTable::testInt128(int128_t value) const { +bool HugeintValuesUsingHashTable::testInt128(const int128_t& value) const { return values_.contains(value); } @@ -1469,7 +1469,7 @@ bool MultiRange::testBytes(const char* value, int32_t length) const { return false; } -bool MultiRange::testTimestamp(Timestamp timestamp) const { +bool MultiRange::testTimestamp(const Timestamp& timestamp) const { for (const auto& filter : filters_) { if (filter->testTimestamp(timestamp)) { return true; diff --git a/velox/type/Filter.h b/velox/type/Filter.h index e8b249a0638a..ac382a7f8379 100644 --- a/velox/type/Filter.h +++ b/velox/type/Filter.h @@ -151,7 +151,7 @@ class Filter : public velox::ISerializable { VELOX_UNSUPPORTED("{}: testInt64() is not supported.", toString()); } - virtual bool testInt128(int128_t /* unused */) const { + virtual bool testInt128(const int128_t& /* unused */) const { VELOX_UNSUPPORTED("{}: testInt128() is not supported.", toString()); } @@ -191,7 +191,7 @@ class Filter : public velox::ISerializable { VELOX_UNSUPPORTED("{}: testBytes() is not supported.", toString()); } - virtual bool testTimestamp(Timestamp /* unused */) const { + virtual bool testTimestamp(const Timestamp& /* unused */) const { VELOX_UNSUPPORTED("{}: testTimestamp() is not supported.", toString()); } @@ -229,8 +229,10 @@ class Filter : public velox::ISerializable { VELOX_UNSUPPORTED("{}: testInt64Range() is not supported.", toString()); } - virtual bool - testInt128Range(int128_t /*min*/, int128_t /*max*/, bool /*hasNull*/) const { + virtual bool testInt128Range( + const int128_t& /*min*/, + const int128_t& /*max*/, + bool /*hasNull*/) const { VELOX_UNSUPPORTED("{}: testInt128Range() is not supported.", toString()); } @@ -250,8 +252,8 @@ class Filter : public velox::ISerializable { } virtual bool testTimestampRange( - Timestamp /*min*/, - Timestamp /*max*/, + const Timestamp& /*min*/, + const Timestamp& /*max*/, bool /*hasNull*/) const { VELOX_UNSUPPORTED("{}: testTimestampRange() is not supported.", toString()); } @@ -323,12 +325,14 @@ class AlwaysFalse final : public Filter { return false; } - bool testInt128Range(int128_t /*min*/, int128_t /*max*/, bool /*hasNull*/) - const final { + bool testInt128Range( + const int128_t& /*min*/, + const int128_t& /*max*/, + bool /*hasNull*/) const final { return false; } - bool testInt128(int128_t /* unused */) const final { + bool testInt128(const int128_t& /* unused */) const final { return false; } @@ -361,8 +365,8 @@ class AlwaysFalse final : public Filter { } bool testTimestampRange( - Timestamp /*min*/, - Timestamp /*max*/, + const Timestamp& /*min*/, + const Timestamp& /*max*/, bool /*hasNull*/) const final { return false; } @@ -407,7 +411,7 @@ class AlwaysTrue final : public Filter { return true; } - bool testInt128(int128_t /* unused */) const final { + bool testInt128(const int128_t& /* unused */) const final { return true; } @@ -416,8 +420,10 @@ class AlwaysTrue final : public Filter { return true; } - bool testInt128Range(int128_t /*min*/, int128_t /*max*/, bool /*hasNull*/) - const final { + bool testInt128Range( + const int128_t& /*min*/, + const int128_t& /*max*/, + bool /*hasNull*/) const final { return true; } @@ -450,8 +456,8 @@ class AlwaysTrue final : public Filter { } bool testTimestampRange( - Timestamp /*min*/, - Timestamp /*max*/, + const Timestamp& /*min*/, + const Timestamp& /*max*/, bool /*hasNull*/) const final { return true; } @@ -492,7 +498,7 @@ class IsNull final : public Filter { return false; } - bool testInt128(int128_t /* unused */) const final { + bool testInt128(const int128_t& /* unused */) const final { return false; } @@ -501,8 +507,10 @@ class IsNull final : public Filter { return hasNull; } - bool testInt128Range(int128_t /*min*/, int128_t /*max*/, bool hasNull) - const final { + bool testInt128Range( + const int128_t& /*min*/, + const int128_t& /*max*/, + bool hasNull) const final { return hasNull; } @@ -527,7 +535,7 @@ class IsNull final : public Filter { return false; } - bool testTimestamp(Timestamp /* unused */) const final { + bool testTimestamp(const Timestamp& /* unused */) const final { return false; } @@ -538,8 +546,10 @@ class IsNull final : public Filter { return hasNull; } - bool testTimestampRange(Timestamp /*min*/, Timestamp /*max*/, bool hasNull) - const final { + bool testTimestampRange( + const Timestamp& /*min*/, + const Timestamp& /*max*/, + bool hasNull) const final { return hasNull; } @@ -576,7 +586,7 @@ class IsNotNull final : public Filter { return true; } - bool testInt128(int128_t /* unused */) const final { + bool testInt128(const int128_t& /* unused */) const final { return true; } @@ -585,8 +595,10 @@ class IsNotNull final : public Filter { return true; } - bool testInt128Range(int128_t /*min*/, int128_t /*max*/, bool /*hasNull*/) - const final { + bool testInt128Range( + const int128_t& /*min*/, + const int128_t& /*max*/, + bool /*hasNull*/) const final { return true; } @@ -611,7 +623,7 @@ class IsNotNull final : public Filter { return true; } - bool testTimestamp(Timestamp /* unused */) const final { + bool testTimestamp(const Timestamp& /* unused */) const final { return true; } @@ -623,8 +635,8 @@ class IsNotNull final : public Filter { } bool testTimestampRange( - Timestamp /*min*/, - Timestamp /*max*/, + const Timestamp& /*min*/, + const Timestamp& /*max*/, bool /*hasNull*/) const final { return true; } @@ -779,7 +791,7 @@ class BigintRange final : public Filter { std::unique_ptr mergeWith(const Filter* other) const final; - std::string toString() const final { + std::string toString() const override { return fmt::format( "BigintRange: [{}, {}] {}", lower_, @@ -858,7 +870,7 @@ class NegatedBigintRange final : public Filter { std::unique_ptr mergeWith(const Filter* other) const final; - std::string toString() const final { + std::string toString() const override { return "Negated" + nonNegated_->toString(); } @@ -873,7 +885,7 @@ class HugeintRange final : public Filter { /// @param lower Lowest value in the rejected range, inclusive. /// @param upper Highest value in the range, inclusive. /// @param nullAllowed Null values are passing the filter if true. - HugeintRange(int128_t lower, int128_t upper, bool nullAllowed) + HugeintRange(const int128_t& lower, const int128_t& upper, bool nullAllowed) : Filter(true, nullAllowed, FilterKind::kHugeintRange), lower_(lower), upper_(upper) {} @@ -892,11 +904,12 @@ class HugeintRange final : public Filter { } } - bool testInt128(int128_t value) const final { + bool testInt128(const int128_t& value) const final { return value >= lower_ && value <= upper_; } - bool testInt128Range(int128_t min, int128_t max, bool hasNull) const final { + bool testInt128Range(const int128_t& min, const int128_t& max, bool hasNull) + const final { if (hasNull && nullAllowed_) { return true; } @@ -912,7 +925,7 @@ class HugeintRange final : public Filter { return upper_; } - std::string toString() const final { + std::string toString() const override { return fmt::format( "HugeintRange: [{}, {}] {}", lower_, @@ -993,7 +1006,7 @@ class BigintValuesUsingHashTable final : public Filter { return hashTable_; } - std::string toString() const final { + std::string toString() const override { return fmt::format( "BigintValuesUsingHashTable: [{}, {}] {}", min_, @@ -1023,8 +1036,8 @@ class BigintValuesUsingHashTable final : public Filter { class HugeintValuesUsingHashTable final : public Filter { public: HugeintValuesUsingHashTable( - const int128_t min, - const int128_t max, + const int128_t& min, + const int128_t& max, const std::vector& values, const bool nullAllowed); @@ -1050,9 +1063,9 @@ class HugeintValuesUsingHashTable final : public Filter { } } - bool testInt128(int128_t value) const final; + bool testInt128(const int128_t& value) const final; - bool testingEquals(const Filter& other) const override; + bool testingEquals(const Filter& other) const final; private: const int128_t min_; @@ -1180,7 +1193,7 @@ class NegatedBigintValuesUsingHashTable final : public Filter { return nonNegated_->values(); } - std::string toString() const final { + std::string toString() const override { return fmt::format( "NegatedBigintValuesUsingHashTable: [{}, {}] {}", nonNegated_->min(), @@ -1456,7 +1469,7 @@ class FloatingPointRange final : public AbstractRange { } } - std::string toString() const final; + std::string toString() const override; bool testingEquals(const Filter& other) const final; @@ -1644,7 +1657,7 @@ class BytesRange final : public AbstractRange { } } - std::string toString() const final { + std::string toString() const override { return fmt::format( "BytesRange: {}{}, {}{} {}", (lowerUnbounded_ || lowerExclusive_) ? "(" : "[", @@ -1752,7 +1765,7 @@ class NegatedBytesRange final : public Filter { *this, nullAllowed.value_or(nullAllowed_)); } - std::string toString() const final { + std::string toString() const override { return "Negated" + nonNegated_->toString(); } @@ -1840,7 +1853,7 @@ class TimestampRange : public Filter { } } - std::string toString() const final { + std::string toString() const override { return fmt::format( "TimestampRange: [{}, {}] {}", lower_.toString(), @@ -1848,12 +1861,14 @@ class TimestampRange : public Filter { nullAllowed_ ? "with nulls" : "no nulls"); } - bool testTimestamp(Timestamp value) const override { + bool testTimestamp(const Timestamp& value) const final { return value >= lower_ && value <= upper_; } - bool testTimestampRange(Timestamp min, Timestamp max, bool hasNull) - const final { + bool testTimestampRange( + const Timestamp& min, + const Timestamp& max, + bool hasNull) const final { if (hasNull && nullAllowed_) { return true; } @@ -2087,7 +2102,7 @@ class MultiRange final : public Filter { bool testBytes(const char* value, int32_t length) const final; - bool testTimestamp(Timestamp value) const final; + bool testTimestamp(const Timestamp& value) const final; bool testLength(int32_t length) const final; @@ -2102,7 +2117,7 @@ class MultiRange final : public Filter { return filters_; } - std::unique_ptr mergeWith(const Filter* other) const override final; + std::unique_ptr mergeWith(const Filter* other) const final; bool testingEquals(const Filter& other) const final;