Skip to content

Commit

Permalink
fix: Fix ODR violation coming from DuckLogicalOperator.h (#12016)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #12016

DuckLogicalOperator.h defines a bunch of DuckDB logical plan nodes that would come from
duckdb-internal.hpp but we can't include that file.

#11930 added LogicalOrder and included a minimal version of
the definition in DuckDB 0.8.1 not including a bunch of fields/functions we don't use. I see minimal versions
of other plan nodes are likewise used in the file, but for whatever reason, this one in particular is tripping up
ODR issue detection during linking when UBSan is enabled.

The fix I offer here is to copy the definition verbatim from duckdb/planner/operator/logical_order.hpp.

I also noticed some unnecessary includes in velox/parse/QueryPlanner.cpp introduced by that PR while
debugging which I've removed here for cleanliness, they're not critical to the fix.

Reviewed By: xiaoxmeng, kgpai

Differential Revision: D67804092

fbshipit-source-id: 7f3363c0bdd340d180da431222f5e7b1b1913f4f
  • Loading branch information
Kevin Wilfong authored and facebook-github-bot committed Jan 3, 2025
1 parent 37292fa commit 8b3b55a
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 10 deletions.
40 changes: 33 additions & 7 deletions velox/parse/DuckLogicalOperator.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,39 @@ class LogicalLimit : public LogicalOperator {
};

class LogicalOrder : public LogicalOperator {
public:
static constexpr const LogicalOperatorType TYPE =
LogicalOperatorType::LOGICAL_ORDER_BY;

public:
explicit LogicalOrder(vector<BoundOrderByNode> orders)
: LogicalOperator(LogicalOperatorType::LOGICAL_ORDER_BY),
orders(std::move(orders)) {}

vector<BoundOrderByNode> orders;
vector<idx_t> projections;

public:
vector<ColumnBinding> GetColumnBindings() override {
auto child_bindings = children[0]->GetColumnBindings();
if (projections.empty()) {
return child_bindings;
}

vector<ColumnBinding> result;
for (auto& col_idx : projections) {
result.push_back(child_bindings[col_idx]);
}
return result;
}

void Serialize(FieldWriter& writer) const override;
static unique_ptr<LogicalOperator> Deserialize(
LogicalDeserializationState& state,
FieldReader& reader);

string ParamsToString() const override {
string result;
string result = "ORDERS:\n";
for (idx_t i = 0; i < orders.size(); i++) {
if (i > 0) {
result += "\n";
Expand All @@ -244,14 +268,16 @@ class LogicalOrder : public LogicalOperator {
return result;
}

public:
vector<ColumnBinding> GetColumnBindings() override {
return children[0]->GetColumnBindings();
}

protected:
void ResolveTypes() override {
types = children[0]->types;
const auto child_types = children[0]->types;
if (projections.empty()) {
types = child_types;
} else {
for (auto& col_idx : projections) {
types.push_back(child_types[col_idx]);
}
}
}
};

Expand Down
3 changes: 0 additions & 3 deletions velox/parse/QueryPlanner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

#include "velox/parse/QueryPlanner.h"
#include <duckdb.hpp> // @manual
#include "velox/duckdb/conversion/DuckConversion.h"
#include "velox/expression/ScopedVarSetter.h"
#include "velox/parse/DuckLogicalOperator.h"
Expand All @@ -29,8 +28,6 @@
#include <duckdb/planner/expression/bound_function_expression.hpp> // @manual
#include <duckdb/planner/expression/bound_reference_expression.hpp> // @manual

#include <iostream>

namespace facebook::velox::core {

namespace {
Expand Down

0 comments on commit 8b3b55a

Please sign in to comment.