From ffb483ac8709c686dda783c533a7390aee566f30 Mon Sep 17 00:00:00 2001 From: Krishna Pai Date: Mon, 16 Dec 2024 14:17:56 -0800 Subject: [PATCH] fix(parquet) : Add support for weak functions to register parquet reader/writers Summary: This diff marks the registerParquet*factory functions with the weak attribute. The weak attribute allows the linker to discard the weaker implementation of the registerParquet*factory functions and in place use the stronger one (See: https://en.wikipedia.org/wiki/Weak_symbol). This should remove the need to compile the same files with different options, leading to ODRs when both targets are added as dependencies to the same binary. Differential Revision: D67301778 --- velox/dwio/parquet/CMakeLists.txt | 4 +-- velox/dwio/parquet/RegisterParquetReader.cpp | 16 ++-------- velox/dwio/parquet/RegisterParquetReader.h | 6 ++-- .../parquet/RegisterParquetReaderImpl.cpp | 31 +++++++++++++++++++ velox/dwio/parquet/RegisterParquetWriter.cpp | 16 ++-------- velox/dwio/parquet/RegisterParquetWriter.h | 6 ++-- .../parquet/RegisterParquetWriterImpl.cpp | 29 +++++++++++++++++ .../tests/reader/ParquetTableScanTest.cpp | 4 +-- 8 files changed, 76 insertions(+), 36 deletions(-) create mode 100644 velox/dwio/parquet/RegisterParquetReaderImpl.cpp create mode 100644 velox/dwio/parquet/RegisterParquetWriterImpl.cpp diff --git a/velox/dwio/parquet/CMakeLists.txt b/velox/dwio/parquet/CMakeLists.txt index 2baaaeee11f5..ede634bed88b 100644 --- a/velox/dwio/parquet/CMakeLists.txt +++ b/velox/dwio/parquet/CMakeLists.txt @@ -23,8 +23,8 @@ if(VELOX_ENABLE_PARQUET) endif() endif() -velox_add_library(velox_dwio_parquet_reader RegisterParquetReader.cpp) -velox_add_library(velox_dwio_parquet_writer RegisterParquetWriter.cpp) +velox_add_library(velox_dwio_parquet_reader RegisterParquetReaderImpl.cpp) +velox_add_library(velox_dwio_parquet_writer RegisterParquetWriterImpl.cpp) if(VELOX_ENABLE_PARQUET) velox_link_libraries(velox_dwio_parquet_reader diff --git a/velox/dwio/parquet/RegisterParquetReader.cpp b/velox/dwio/parquet/RegisterParquetReader.cpp index 2f4cc94b81fe..2869109a45bc 100644 --- a/velox/dwio/parquet/RegisterParquetReader.cpp +++ b/velox/dwio/parquet/RegisterParquetReader.cpp @@ -14,22 +14,10 @@ * limitations under the License. */ -#ifdef VELOX_ENABLE_PARQUET -#include "velox/dwio/parquet/reader/ParquetReader.h" // @manual -#endif - namespace facebook::velox::parquet { -void registerParquetReaderFactory() { -#ifdef VELOX_ENABLE_PARQUET - dwio::common::registerReaderFactory(std::make_shared()); -#endif -} +void registerParquetReaderFactory() {} -void unregisterParquetReaderFactory() { -#ifdef VELOX_ENABLE_PARQUET - dwio::common::unregisterReaderFactory(dwio::common::FileFormat::PARQUET); -#endif -} +void unregisterParquetReaderFactory() {} } // namespace facebook::velox::parquet diff --git a/velox/dwio/parquet/RegisterParquetReader.h b/velox/dwio/parquet/RegisterParquetReader.h index af7781571660..26bf7cb80233 100644 --- a/velox/dwio/parquet/RegisterParquetReader.h +++ b/velox/dwio/parquet/RegisterParquetReader.h @@ -18,8 +18,10 @@ namespace facebook::velox::parquet { -void registerParquetReaderFactory(); +/// Registers the Parquet reader factory. +/// Has the weak attribute so that it can be overridden by a custom factory. +__attribute__((__weak__)) void registerParquetReaderFactory(); -void unregisterParquetReaderFactory(); +__attribute__((__weak__)) void unregisterParquetReaderFactory(); } // namespace facebook::velox::parquet diff --git a/velox/dwio/parquet/RegisterParquetReaderImpl.cpp b/velox/dwio/parquet/RegisterParquetReaderImpl.cpp new file mode 100644 index 000000000000..0ae48e215d9c --- /dev/null +++ b/velox/dwio/parquet/RegisterParquetReaderImpl.cpp @@ -0,0 +1,31 @@ +/* + * 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. + */ + +// This file invokes the Parquet Reader + +#include "velox/dwio/parquet/reader/ParquetReader.h" + +namespace facebook::velox::parquet { + +void registerParquetReaderFactory() { + dwio::common::registerReaderFactory(std::make_shared()); +} + +void unregisterParquetReaderFactory() { + dwio::common::unregisterReaderFactory(dwio::common::FileFormat::PARQUET); +} + +} // namespace facebook::velox::parquet diff --git a/velox/dwio/parquet/RegisterParquetWriter.cpp b/velox/dwio/parquet/RegisterParquetWriter.cpp index 1d6c0e033357..9494434f1e64 100644 --- a/velox/dwio/parquet/RegisterParquetWriter.cpp +++ b/velox/dwio/parquet/RegisterParquetWriter.cpp @@ -14,22 +14,10 @@ * limitations under the License. */ -#ifdef VELOX_ENABLE_PARQUET -#include "velox/dwio/parquet/writer/Writer.h" // @manual -#endif - namespace facebook::velox::parquet { -void registerParquetWriterFactory() { -#ifdef VELOX_ENABLE_PARQUET - dwio::common::registerWriterFactory(std::make_shared()); -#endif -} +void registerParquetWriterFactory() {} -void unregisterParquetWriterFactory() { -#ifdef VELOX_ENABLE_PARQUET - dwio::common::unregisterWriterFactory(dwio::common::FileFormat::PARQUET); -#endif -} +void unregisterParquetWriterFactory() {} } // namespace facebook::velox::parquet diff --git a/velox/dwio/parquet/RegisterParquetWriter.h b/velox/dwio/parquet/RegisterParquetWriter.h index ff14417d8a5d..6b7f7af0ba97 100644 --- a/velox/dwio/parquet/RegisterParquetWriter.h +++ b/velox/dwio/parquet/RegisterParquetWriter.h @@ -18,8 +18,10 @@ namespace facebook::velox::parquet { -void registerParquetWriterFactory(); +/// Registers the Parquet writer factory. +/// Has the weak attribute so that it can be overridden by a custom factory. +__attribute__((__weak__)) void registerParquetWriterFactory(); -void unregisterParquetWriterFactory(); +__attribute__((__weak__)) void unregisterParquetWriterFactory(); } // namespace facebook::velox::parquet diff --git a/velox/dwio/parquet/RegisterParquetWriterImpl.cpp b/velox/dwio/parquet/RegisterParquetWriterImpl.cpp new file mode 100644 index 000000000000..bff76dc3fb63 --- /dev/null +++ b/velox/dwio/parquet/RegisterParquetWriterImpl.cpp @@ -0,0 +1,29 @@ +/* + * 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. + */ + +#include "velox/dwio/parquet/writer/Writer.h" + +namespace facebook::velox::parquet { + +void registerParquetWriterFactory() { + dwio::common::registerWriterFactory(std::make_shared()); +} + +void unregisterParquetWriterFactory() { + dwio::common::unregisterWriterFactory(dwio::common::FileFormat::PARQUET); +} + +} // namespace facebook::velox::parquet diff --git a/velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp b/velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp index 20826137cba1..06cfb0977582 100644 --- a/velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp +++ b/velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp @@ -20,7 +20,7 @@ #include "velox/dwio/common/tests/utils/DataFiles.h" // @manual #include "velox/dwio/parquet/RegisterParquetReader.h" // @manual #include "velox/dwio/parquet/reader/PageReader.h" // @manual -#include "velox/dwio/parquet/reader/ParquetReader.h" // @manual=//velox/connectors/hive:velox_hive_connector_parquet +#include "velox/dwio/parquet/reader/ParquetReader.h" #include "velox/exec/tests/utils/AssertQueryBuilder.h" #include "velox/exec/tests/utils/HiveConnectorTestBase.h" // @manual #include "velox/exec/tests/utils/PlanBuilder.h" @@ -29,7 +29,7 @@ #include "velox/type/tests/SubfieldFiltersBuilder.h" #include "velox/type/tz/TimeZoneMap.h" -#include "velox/connectors/hive/HiveConfig.h" // @manual=//velox/connectors/hive:velox_hive_connector_parquet +#include "velox/connectors/hive/HiveConfig.h" #include "velox/dwio/parquet/writer/Writer.h" // @manual using namespace facebook::velox;