Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: GCC13 build failures #12517

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: GCC13 build failures #12517

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da01a1e
Fix GCC13 build failures.
xin-zhang2 Mar 4, 2025
d6456a5
Fix format issue.
Mar 6, 2025
9670597
address review issues
Mar 6, 2025
3c052e2
A temporary comment to trigger GCC 13 build.
xin-zhang2 Mar 10, 2025
9addb45
Revert "A temporary comment to trigger GCC 13 build."
xin-zhang2 Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions velox/common/config/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.

if (${VELOX_BUILD_TESTING})
if(${VELOX_BUILD_TESTING})
add_subdirectory(tests)
endif ()
endif()

velox_add_library(velox_common_config Config.cpp)
velox_link_libraries(
velox_common_config
PUBLIC velox_common_base
velox_exception
PUBLIC velox_common_base velox_exception
PRIVATE re2::re2)
7 changes: 1 addition & 6 deletions velox/connectors/tpch/TpchConnector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,7 @@ TpchDataSource::TpchDataSource(
toTableName(tpchTable_));

auto handle = std::dynamic_pointer_cast<TpchColumnHandle>(it->second);
VELOX_CHECK_NOT_NULL(
handle,
"ColumnHandle must be an instance of TpchColumnHandle "
"for '{}' on table '{}'",
handle->name(),
toTableName(tpchTable_));
VELOX_CHECK_NOT_NULL(handle, "ColumnHandle can't be null.");

auto idx = tpchTableSchema->getChildIdxIfExists(handle->name());
VELOX_CHECK(
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/dwrf/common/Compression.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ inline std::unique_ptr<dwio::common::BufferedOutputStream> createCompressor(

inline CompressionOptions getDwrfOrcDecompressionOptions(
common::CompressionKind kind) {
CompressionOptions options;
CompressionOptions options{};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add {} here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialize options explicitly to avoid using it uninitialized.

/root/velox/./velox/dwio/dwrf/common/Compression.h:99:10: error: ‘options’ is used uninitialized [-Werror=uninitialized]
   99 |   return options;
      |          ^~~~~~~
/root/velox/./velox/dwio/dwrf/common/Compression.h: In member function ‘virtual void TestSeek_uncompressed_Test::TestBody()’:
/root/velox/./velox/dwio/dwrf/common/Compression.h:90:22: note: ‘options’ declared here
   90 |   CompressionOptions options;
      |                      ^~~~~~~

Copy link
Collaborator

@majetideepak majetideepak Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's initialize the fields in the struct CompressionOptions
example: uint32_t compressionThreshold; must be uint32_t compressionThreshold{0};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialized the fields.
There is no default value for CompressionOptions.format, so I left it empty initialized, which results in format.zlib.windowBits and format.zlib.compressionLevel being initialized to 0.
As for CompressionOptions.compressionThreshold, it is currently used only by DwrfOrcCompression, where its value is configured via orc.compression.threshold, with a default of 256. However, since CompressionOptions is part of the common package, I believe it would be more appropriate to initialize compressionThreshold to 0.
Does the approach seem reasonable?

if (kind == common::CompressionKind_ZLIB ||
kind == common::CompressionKind_GZIP) {
options.format.zlib.windowBits = Compressor::DWRF_ORC_ZLIB_WINDOW_BITS;
Expand Down
6 changes: 5 additions & 1 deletion velox/exec/tests/ContainerRowSerdeTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,11 @@ TEST_F(ContainerRowSerdeTest, nested) {
testRoundTrip(data);

auto nestedArray = makeNullableNestedArrayVector<std::string>(
{{{{{"1", "2"}}, {{"3", "4"}}}}, {{}}, {{std::nullopt, {}}}});
{{{{{"1", "2"}}, {{"3", "4"}}}},
std::make_optional<
std::vector<std::optional<std::vector<std::optional<std::string>>>>>(
{}),
{{std::nullopt, {}}}});

testRoundTrip(nestedArray);

Expand Down
2 changes: 1 addition & 1 deletion velox/exec/tests/UnnestTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ TEST_P(UnnestTest, arrayWithOrdinality) {
{{{1, 2, std::nullopt, 4}},
std::nullopt,
{{5, 6}},
{{}},
std::make_optional<std::vector<std::optional<int32_t>>>({}),
{{{{std::nullopt}}}},
{{7, 8, 9}}});
auto vector = makeRowVector(
Expand Down
2 changes: 1 addition & 1 deletion velox/expression/tests/ArrayWriterTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ TEST_F(ArrayWriterTest, copyFromNestedArray) {
vectorWriter.finish();

using array_type = std::optional<std::vector<std::optional<int64_t>>>;
array_type array1 = {{}};
array_type array1 = std::make_optional<array_type::value_type>({});
array_type array2 = {{1, 2, 3, 4}};
array_type array3 = {{1}};

Expand Down
5 changes: 1 addition & 4 deletions velox/expression/tests/ExpressionRunner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,7 @@ void ExpressionRunner::run(
auto inputVector =
std::dynamic_pointer_cast<RowVector>(restoreVectorFromFile(
inputPathsList[i].c_str(), deserializerPool.get()));
VELOX_CHECK_NOT_NULL(
inputVector,
"Input vector is not a RowVector: {}",
inputVector->toString());
VELOX_CHECK_NOT_NULL(inputVector, "Input vector can't be null.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When inputVector is nullptr, inputVector->toString() in the previous message will cause a null pointer dereference.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

auto vector = restoreVectorFromFile(inputPathsList[i].c_str(), deserializerPool.get());
auto inputVector = std::dynamic_pointer_cast<RowVector>(vector);

use vector->toString() in the null check argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

VELOX_CHECK_GT(inputVector->size(), 0, "Input vector must not be empty.");
if (inputSelectivityPaths.size() > i) {
inputTestCases.push_back(
Expand Down
4 changes: 2 additions & 2 deletions velox/expression/tests/GenericViewTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class GenericViewTest : public functions::test::FunctionBaseTest {
std::vector<std::optional<std::vector<std::optional<int64_t>>>>;

array_data_t arrayData1 = {
{{}},
std::make_optional<std::vector<std::optional<int64_t>>>({}),
{{{{std::nullopt}}}},
{{std::nullopt, 1}},
{{std::nullopt, std::nullopt, std::nullopt}},
Expand All @@ -56,7 +56,7 @@ class GenericViewTest : public functions::test::FunctionBaseTest {
};

array_data_t arrayData2 = {
{{}},
std::make_optional<std::vector<std::optional<int64_t>>>({}),
{{{{std::nullopt}}}},
{{std::nullopt, 1}},
{{std::nullopt, std::nullopt, std::nullopt}},
Expand Down
22 changes: 17 additions & 5 deletions velox/functions/lib/tests/Re2FunctionsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,19 +1223,31 @@ TEST_F(Re2FunctionsTest, regexExtractAllNoMatch) {
const std::vector<std::optional<int32_t>> noGroupId = {};
const std::vector<std::optional<int32_t>> groupIds0 = {0};

testRe2ExtractAll({""}, {"[0-9]+"}, noGroupId, {{{}}});
testRe2ExtractAll({"(╯°□°)╯︵ ┻━┻"}, {"[0-9]+"}, noGroupId, {{{}}});
testRe2ExtractAll({"abcde"}, {"[0-9]+"}, groupIds0, {{{}}});
testRe2ExtractAll(
{""},
{"[0-9]+"},
noGroupId,
{std::make_optional<std::vector<std::string>>({})});
testRe2ExtractAll(
{"(╯°□°)╯︵ ┻━┻"},
{"[0-9]+"},
noGroupId,
{std::make_optional<std::vector<std::string>>({})});
testRe2ExtractAll(
{"abcde"},
{"[0-9]+"},
groupIds0,
{std::make_optional<std::vector<std::string>>({})});
testRe2ExtractAll(
{"rYBKVn6DnfSI2an4is4jbvf4btGpV"},
{"81jnp58n31BtMdlUsP1hiF4QWSYv411"},
noGroupId,
{{{}}});
{std::make_optional<std::vector<std::string>>({})});
testRe2ExtractAll(
{"rYBKVn6DnfSI2an4is4jbvf4btGpV"},
{"81jnp58n31BtMdlUsP1hiF4QWSYv411"},
groupIds0,
{{{}}});
{std::make_optional<std::vector<std::string>>({})});

// Test empty pattern.
testRe2ExtractAll<int32_t>(
Expand Down
2 changes: 1 addition & 1 deletion velox/functions/lib/tests/RepeatTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ TEST_F(RepeatTest, repeat) {
{{0.0}},
{{-2.0, -2.0}},
{{3.333333, 3.333333, 3.333333}},
{{}},
std::make_optional<std::vector<std::optional<float>>>({}),
{{std::nullopt, std::nullopt, std::nullopt, std::nullopt}},
std::nullopt,
});
Expand Down
6 changes: 4 additions & 2 deletions velox/functions/prestosql/aggregates/tests/ChecksumAggregateTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,10 @@ TEST_F(ChecksumAggregateTest, arrays) {
arrayVector = makeNullableArrayVector<int64_t>({{{1, 2}}, std::nullopt});
assertChecksum(arrayVector, "Nlzernkj88A=");

arrayVector =
makeNullableArrayVector<int64_t>({{{1, 2}}, std::nullopt, {{}}});
arrayVector = makeNullableArrayVector<int64_t>(
{{{1, 2}},
std::nullopt,
std::make_optional<std::vector<std::optional<int64_t>>>({})});
assertChecksum(arrayVector, "Nlzernkj88A=");

// Array of arrays.
Expand Down
2 changes: 1 addition & 1 deletion velox/functions/prestosql/aggregates/tests/ClassificationAggregationTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ TEST_F(ClassificationAggregationTest, basic) {
});

expected = makeRowVector({makeNullableArrayVector<double>(
std::vector<std::optional<std::vector<std::optional<double>>>>{{{}}})});
{{std::make_optional<std::vector<std::optional<double>>>({})}})});
runTest("classification_fall_out(5, c0, c1)", input, expected);
runTest("classification_precision(5, c0, c1)", input, expected);
runTest("classification_recall(5, c0, c1)", input, expected);
Expand Down
3 changes: 2 additions & 1 deletion velox/functions/prestosql/aggregates/tests/MapUnionAggregationTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ TEST_F(MapUnionTest, nulls) {
makeNullableMapVector<int64_t, int64_t>({
{{{1, 10}, {2, 20}, {3, 33}, {4, 44}, {5, 55}}},
std::nullopt,
{{}},
std::make_optional<
std::vector<std::pair<int64_t, std::optional<int64_t>>>>({}),
}),
});

Expand Down
14 changes: 10 additions & 4 deletions velox/functions/prestosql/aggregates/tests/MapUnionSumTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ class MapUnionSumTest : public AggregationTestBase {};
TEST_F(MapUnionSumTest, global) {
auto data = makeRowVector({
makeNullableMapVector<int64_t, int64_t>({
{{}}, // empty map
std::make_optional<
std::vector<std::pair<int64_t, std::optional<int64_t>>>>(
{}), // empty map
std::nullopt, // null map
{{{1, 10}, {2, 20}}},
{{{1, 11}, {3, 30}, {4, 40}}},
Expand Down Expand Up @@ -62,7 +64,9 @@ TEST_F(MapUnionSumTest, globalVarcharKey) {

auto data = makeRowVector({
makeNullableMapVector<StringView, int64_t>({
{{}}, // empty map
std::make_optional<
std::vector<std::pair<StringView, std::optional<int64_t>>>>(
{}), // empty map
std::nullopt, // null map
{{{keys[0], 10}, {keys[1], 20}}},
{{{keys[0], 11}, {keys[2], 30}, {keys[3], 40}}},
Expand Down Expand Up @@ -119,9 +123,11 @@ TEST_F(MapUnionSumTest, nullAndEmptyMaps) {
auto emptyAndNullMaps = makeRowVector({
makeNullableMapVector<int64_t, int64_t>({
std::nullopt,
{{}},
std::make_optional<
std::vector<std::pair<int64_t, std::optional<int64_t>>>>({}),
std::nullopt,
{{}},
std::make_optional<
std::vector<std::pair<int64_t, std::optional<int64_t>>>>({}),
}),
});

Expand Down
4 changes: 2 additions & 2 deletions velox/functions/prestosql/aggregates/tests/PrestoHasherTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ TEST_F(PrestoHasherTest, arrays) {
{{10, 11}},
{{12, std::nullopt}},
std::nullopt,
{{}}});
std::make_optional<std::vector<std::optional<int64_t>>>({})});

assertHash(
baseArrayVector,
Expand All @@ -320,7 +320,7 @@ TEST_F(PrestoHasherTest, arrays) {
{{std::nullopt}},
{{1, 2, 3}},
{{1024, std::nullopt, -99, -999}},
{{}},
std::make_optional<std::vector<std::optional<int64_t>>>({}),
{{std::nullopt, -1}},
});

Expand Down
5 changes: 3 additions & 2 deletions velox/functions/prestosql/aggregates/tests/SetAggTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ TEST_F(SetAggTest, arrayNestedNulls) {
});

auto expected = makeRowVector({makeNullableNestedArrayVector<int32_t>(
{{{{{}},
{{{std::make_optional<std::vector<std::optional<int32_t>>>({}),
{{1, 2}},
{{2, 3}},
{{3, std::nullopt}},
Expand All @@ -544,7 +544,8 @@ TEST_F(SetAggTest, arrayNestedNulls) {
expected = makeRowVector(
{makeFlatVector<int32_t>({1, 2, 3, 4}),
makeNullableNestedArrayVector<int32_t>(
{{{{{{}}, {{1, 2}}}},
{{{{std::make_optional<std::vector<std::optional<int32_t>>>({}),
{{1, 2}}}},
{{{{6, 7}}, {{std::nullopt, 7}}}},
{{{{2, 3}}, {{3, std::nullopt}}}},
{{{{8, 9}}, {{std::nullopt, std::nullopt}}}}}})});
Expand Down
18 changes: 12 additions & 6 deletions velox/functions/prestosql/tests/ArrayCombinationsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ class ArrayCombinationsTest : public FunctionBaseTest {
{{{{std::vector<std::optional<T>>()}}},
{{{{0, 1, 2}}, {{0, 1, 3}}, {{0, 2, 3}}, {{1, 2, 3}}}},
{{{{0, 1, 2, 3}}}},
{{}}});
std::make_optional<
std::vector<std::optional<std::vector<std::optional<T>>>>>({})});
testExpr(
expected, "combinations(C0, C1)", {arrayVector, comboLengthVector});
}
Expand All @@ -68,7 +69,8 @@ class ArrayCombinationsTest : public FunctionBaseTest {
{{
{{0, 1, std::nullopt, 3}},
}},
{{}},
std::make_optional<
std::vector<std::optional<std::vector<std::optional<T>>>>>({}),
});
testExpr(
expected, "combinations(C0, C1)", {arrayVector, comboLengthVector});
Expand Down Expand Up @@ -175,7 +177,8 @@ TEST_F(ArrayCombinationsTest, inlineVarcharArrays) {
{{"bb", "aa", "aa", "ddd"}},
{{"bb", "cc", "aa", "ddd"}},
{{"aa", "cc", "aa", "ddd"}}}},
{{}}});
std::make_optional<
std::vector<std::optional<std::vector<std::optional<S>>>>>({})});
testExpr(expected, "combinations(C0, C1)", {arrayVector, comboLengthVector});
}

Expand Down Expand Up @@ -222,7 +225,8 @@ TEST_F(ArrayCombinationsTest, varcharArrays) {
"yellow rose flowers",
"red shiny car ahead",
"purple is an elegant color"}}}},
{{}}});
std::make_optional<
std::vector<std::optional<std::vector<std::optional<S>>>>>({})});
testExpr(expected, "combinations(C0, C1)", {arrayVector, comboLengthVector});
}

Expand All @@ -245,7 +249,8 @@ TEST_F(ArrayCombinationsTest, boolNullableArrays) {
{{false, true, true, true}},
{{false, false, true, true}},
{{true, false, true, true}}}},
{{}}});
std::make_optional<
std::vector<std::optional<std::vector<std::optional<bool>>>>>({})});
testExpr(expected, "combinations(C0, C1)", {arrayVector, comboLengthVector});
}

Expand All @@ -268,6 +273,7 @@ TEST_F(ArrayCombinationsTest, boolArrays) {
{{false, true, true, true}},
{{false, false, true, true}},
{{true, false, true, true}}}},
{{}}});
std::make_optional<
std::vector<std::optional<std::vector<std::optional<bool>>>>>({})});
testExpr(expected, "combinations(C0, C1)", {arrayVector, comboLengthVector});
}
3 changes: 2 additions & 1 deletion velox/functions/prestosql/tests/ArrayExceptTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ TEST_F(ArrayExceptTest, boolArrays) {
{true, false, true}});

auto expected = makeNullableArrayVector<bool>(
{{false}, {}, {}, {}, {false}, {true}, {}, {}});
std::vector<std::vector<std::optional<bool>>>{
{false}, {}, {}, {}, {false}, {true}, {}, {}});

testExpr(expected, "array_except(C0, C1)", {array1, array2});

Expand Down
Loading
Loading