From c2ba621f3d43a058e457c4b800f3566a473fed28 Mon Sep 17 00:00:00 2001 From: Jimmy Lu Date: Mon, 16 Dec 2024 08:47:57 -0800 Subject: [PATCH] refactor: Remove unused and misleading Tree superclass Summary: The `Tree` superclass is misleading (`size()` only counts direct children and iterator only iterates direct children; usually for such interface we would expect to iterate the whole tree) and not really used anywhere. Remove it to make the code base cleaner. Differential Revision: D67241802 --- velox/dwio/common/TypeWithId.cpp | 17 ++--- velox/dwio/common/TypeWithId.h | 12 +++- velox/type/Tree.h | 109 ------------------------------- velox/type/Type.cpp | 4 +- velox/type/Type.h | 11 +++- velox/type/tests/TypeTest.cpp | 36 ---------- 6 files changed, 23 insertions(+), 166 deletions(-) delete mode 100644 velox/type/Tree.h diff --git a/velox/dwio/common/TypeWithId.cpp b/velox/dwio/common/TypeWithId.cpp index 03328024ef67..0eaf7ee0271c 100644 --- a/velox/dwio/common/TypeWithId.cpp +++ b/velox/dwio/common/TypeWithId.cpp @@ -64,8 +64,8 @@ namespace { int countNodes(const TypePtr& type) { int count = 1; - for (auto& child : *type) { - count += countNodes(child); + for (uint32_t end = type->size(), i = 0; i < end; ++i) { + count += countNodes(type->childAt(i)); } return count; } @@ -89,15 +89,6 @@ std::unique_ptr TypeWithId::create( type, std::move(children), 0, next - 1, 0); } -uint32_t TypeWithId::size() const { - return children_.size(); -} - -const std::shared_ptr& TypeWithId::childAt( - uint32_t idx) const { - return children_.at(idx); -} - std::unique_ptr TypeWithId::create( const std::shared_ptr& type, uint32_t& next, @@ -107,9 +98,9 @@ std::unique_ptr TypeWithId::create( std::vector> children; children.reserve(type->size()); auto offset = 0; - for (const auto& child : *type) { + for (uint32_t end = type->size(), i = 0; i < end; ++i) { children.emplace_back(create( - child, + type->childAt(i), next, (myId == 0 && type->kind() == TypeKind::ROW) ? offset++ : column)); } diff --git a/velox/dwio/common/TypeWithId.h b/velox/dwio/common/TypeWithId.h index a147cfe5066f..0758eb907a61 100644 --- a/velox/dwio/common/TypeWithId.h +++ b/velox/dwio/common/TypeWithId.h @@ -23,7 +23,7 @@ namespace facebook::velox::dwio::common { -class TypeWithId : public velox::Tree> { +class TypeWithId { public: /// NOTE: This constructor will re-parent the children. TypeWithId( @@ -33,6 +33,8 @@ class TypeWithId : public velox::Tree> { uint32_t maxId, uint32_t column); + virtual ~TypeWithId() = default; + TypeWithId(const TypeWithId&) = delete; TypeWithId& operator=(const TypeWithId&) = delete; @@ -47,7 +49,9 @@ class TypeWithId : public velox::Tree> { const RowTypePtr& type, const velox::common::ScanSpec& spec); - uint32_t size() const override; + uint32_t size() const { + return children_.size(); + } const std::shared_ptr& type() const { return type_; @@ -69,7 +73,9 @@ class TypeWithId : public velox::Tree> { return column_; } - const std::shared_ptr& childAt(uint32_t idx) const override; + const std::shared_ptr& childAt(uint32_t idx) const { + return children_.at(idx); + } const std::shared_ptr& childByName( const std::string& name) const { diff --git a/velox/type/Tree.h b/velox/type/Tree.h deleted file mode 100644 index 269553f911e7..000000000000 --- a/velox/type/Tree.h +++ /dev/null @@ -1,109 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -#pragma once - -#include -namespace facebook { -namespace velox { - -template -class Tree; - -template -class TreeIterator { - public: - typedef TreeIterator self_type; - typedef const T value_type; - typedef const T& reference; - typedef const T* pointer; - // todo: support more iterator types - typedef std::forward_iterator_tag iterator_category; - typedef int64_t difference_type; - - TreeIterator(const Tree& tree, size_t idx) : tree_(tree), idx_{idx} {} - - self_type operator++() { - // prefix - idx_++; - return *this; - } - - self_type operator++(int32_t) { - // postfix - self_type i = *this; - idx_++; - return i; - } - - reference operator*() { - return tree_.childAt(idx_); - } - - pointer operator->() { - return &tree_.childAt(idx_); - } - - bool operator==(const self_type& rhs) const { - return &tree_ == &rhs.tree_ && idx_ == rhs.idx_; - } - - bool operator!=(const self_type& rhs) const { - return (&tree_ != &(rhs.tree_)) || idx_ != rhs.idx_; - } - - private: - const Tree& tree_; - size_t idx_; -}; - -template -class Tree { - public: - using const_reference = const T&; - using reference_type = const_reference; - using const_iterator = TreeIterator; - using iterator = const_iterator; - - using difference_type = uint64_t; - using size_type = uint32_t; - - virtual ~Tree() = default; - - virtual size_type size() const = 0; - - virtual const_reference childAt(size_type idx) const = 0; - - const_iterator begin() const { - return cbegin(); - } - - const_iterator end() const { - return cend(); - } - - const_iterator cbegin() const { - return const_iterator{*this, 0}; - } - - const_iterator cend() const { - return const_iterator{*this, size()}; - } - - // todo(youknowjack): remaining container members -}; - -} // namespace velox -} // namespace facebook diff --git a/velox/type/Type.cpp b/velox/type/Type.cpp index 4ede479a3ee7..d617d30e7149 100644 --- a/velox/type/Type.cpp +++ b/velox/type/Type.cpp @@ -530,8 +530,8 @@ folly::dynamic RowType::serialize() const { size_t Type::hashKind() const { size_t hash = (int32_t)kind(); - for (auto& child : *this) { - hash = hash * 31 + child->hashKind(); + for (uint32_t end = size(), i = 0; i < end; ++i) { + hash = hash * 31 + childAt(i)->hashKind(); } return hash; } diff --git a/velox/type/Type.h b/velox/type/Type.h index 65e0f2988fe0..c9f3d319bd09 100644 --- a/velox/type/Type.h +++ b/velox/type/Type.h @@ -37,7 +37,6 @@ #include "velox/type/HugeInt.h" #include "velox/type/StringView.h" #include "velox/type/Timestamp.h" -#include "velox/type/Tree.h" namespace facebook::velox { @@ -433,7 +432,7 @@ struct TypeParameter { /// IntegerType ArrayType /// | /// BigintType -class Type : public Tree, public velox::ISerializable { +class Type : public velox::ISerializable { public: explicit Type(TypeKind kind, bool providesCustomComparison = false) : kind_{kind}, providesCustomComparison_(providesCustomComparison) {} @@ -480,6 +479,12 @@ class Type : public Tree, public velox::ISerializable { /// Returns a possibly empty list of type parameters. virtual const std::vector& parameters() const = 0; + /// Returns the number of children of this type. + virtual uint32_t size() const = 0; + + /// Returns the child at position index. Index must be smaller than size(). + virtual const TypePtr& childAt(uint32_t index) const = 0; + /// Returns physical type name. Multiple logical types may share the same /// physical type backing and therefore return the same physical type name. /// The logical type name returned by 'name()' must be unique though. @@ -490,7 +495,7 @@ class Type : public Tree, public velox::ISerializable { /// Options to control the output of toSummaryString(). struct TypeSummaryOptions { /// Maximum number of child types to include in the summary. - size_type maxChildren{0}; + uint32_t maxChildren{0}; }; /// Returns human-readable summary of the type. Useful when full output of diff --git a/velox/type/tests/TypeTest.cpp b/velox/type/tests/TypeTest.cpp index 8f0678a47e5b..1a584fc4b2aa 100644 --- a/velox/type/tests/TypeTest.cpp +++ b/velox/type/tests/TypeTest.cpp @@ -85,7 +85,6 @@ TEST(TypeTest, integer) { EXPECT_THROW(int0->childAt(0), std::invalid_argument); EXPECT_EQ(int0->kind(), TypeKind::INTEGER); EXPECT_STREQ(int0->kindName(), "INTEGER"); - EXPECT_EQ(int0->begin(), int0->end()); testTypeSerde(int0); } @@ -101,7 +100,6 @@ TEST(TypeTest, timestamp) { EXPECT_THROW(t0->childAt(0), std::invalid_argument); EXPECT_EQ(t0->kind(), TypeKind::TIMESTAMP); EXPECT_STREQ(t0->kindName(), "TIMESTAMP"); - EXPECT_EQ(t0->begin(), t0->end()); testTypeSerde(t0); } @@ -158,7 +156,6 @@ TEST(TypeTest, date) { EXPECT_THROW(date->childAt(0), std::invalid_argument); EXPECT_EQ(date->kind(), TypeKind::INTEGER); EXPECT_STREQ(date->kindName(), "INTEGER"); - EXPECT_EQ(date->begin(), date->end()); EXPECT_TRUE(date->kindEquals(INTEGER())); EXPECT_NE(*date, *INTEGER()); @@ -175,7 +172,6 @@ TEST(TypeTest, intervalDayTime) { EXPECT_THROW(interval->childAt(0), std::invalid_argument); EXPECT_EQ(interval->kind(), TypeKind::BIGINT); EXPECT_STREQ(interval->kindName(), "BIGINT"); - EXPECT_EQ(interval->begin(), interval->end()); EXPECT_TRUE(interval->kindEquals(BIGINT())); EXPECT_NE(*interval, *BIGINT()); @@ -196,7 +192,6 @@ TEST(TypeTest, intervalYearMonth) { EXPECT_THROW(interval->childAt(0), std::invalid_argument); EXPECT_EQ(interval->kind(), TypeKind::INTEGER); EXPECT_STREQ(interval->kindName(), "INTEGER"); - EXPECT_EQ(interval->begin(), interval->end()); EXPECT_TRUE(interval->kindEquals(INTEGER())); EXPECT_NE(*interval, *INTEGER()); @@ -224,7 +219,6 @@ TEST(TypeTest, unknown) { EXPECT_THROW(type->childAt(0), std::invalid_argument); EXPECT_EQ(type->kind(), TypeKind::UNKNOWN); EXPECT_STREQ(type->kindName(), "UNKNOWN"); - EXPECT_EQ(type->begin(), type->end()); EXPECT_TRUE(type->isComparable()); EXPECT_TRUE(type->isOrderable()); @@ -237,7 +231,6 @@ TEST(TypeTest, shortDecimal) { EXPECT_EQ(shortDecimal->size(), 0); EXPECT_THROW(shortDecimal->childAt(0), std::invalid_argument); EXPECT_EQ(shortDecimal->kind(), TypeKind::BIGINT); - EXPECT_EQ(shortDecimal->begin(), shortDecimal->end()); EXPECT_EQ(*DECIMAL(10, 5), *shortDecimal); EXPECT_NE(*DECIMAL(9, 5), *shortDecimal); @@ -273,7 +266,6 @@ TEST(TypeTest, longDecimal) { EXPECT_EQ(longDecimal->size(), 0); EXPECT_THROW(longDecimal->childAt(0), std::invalid_argument); EXPECT_EQ(longDecimal->kind(), TypeKind::HUGEINT); - EXPECT_EQ(longDecimal->begin(), longDecimal->end()); EXPECT_EQ(*DECIMAL(30, 5), *longDecimal); EXPECT_NE(*DECIMAL(9, 5), *longDecimal); EXPECT_NE(*DECIMAL(30, 3), *longDecimal); @@ -370,18 +362,6 @@ TEST(TypeTest, map) { EXPECT_THROW(mapType->childAt(2), VeloxUserError); EXPECT_EQ(mapType->kind(), TypeKind::MAP); EXPECT_STREQ(mapType->kindName(), "MAP"); - int32_t num = 0; - for (auto& i : *mapType) { - if (num == 0) { - EXPECT_EQ(i->toString(), "INTEGER"); - } else if (num == 1) { - EXPECT_EQ(i->toString(), "ARRAY"); - } else { - FAIL(); - } - ++num; - } - CHECK_EQ(num, 2); EXPECT_STREQ(mapType->name(), "MAP"); EXPECT_EQ(mapType->parameters().size(), 2); @@ -424,22 +404,6 @@ TEST(TypeTest, row) { EXPECT_TRUE(row0->containsChild("a")); EXPECT_TRUE(row0->containsChild("b")); EXPECT_FALSE(row0->containsChild("c")); - int32_t seen = 0; - for (auto& i : *row0) { - if (seen == 0) { - EXPECT_STREQ("INTEGER", i->kindName()); - } else if (seen == 1) { - EXPECT_EQ("ROW", i->toString()); - int32_t seen2 = 0; - for (auto& j : *i) { - EXPECT_EQ(j->toString(), "BIGINT"); - seen2++; - } - EXPECT_EQ(seen2, 1); - } - seen++; - } - CHECK_EQ(seen, 2); EXPECT_STREQ(row0->name(), "ROW"); EXPECT_EQ(row0->parameters().size(), 2);