Skip to content

Commit 038c058

Browse files
authored
fix: revert "feat(daft-ext): scalar daft_func macro with overloading (#6844)" (#6925)
## Changes Made This reverts commit afb30af. It's an immediate fix for an extension regression which is blocking the 0.7.11 release. After the release is fixed, we can reintroduce this with a longer-term fix. We could also just remove the overloading things for now and leave the proc macro as an immediate follow-up. ## Related Issues #6922 This hasn't been verified, but a patch would be this. That being said, I'd prefer the longer term fix which uses interior mutability with a scalar function factory to fix the overload registration and allow for overloads for both extensions and daft internal. ```rs if self.variants.len() == 1 { return Ok(BuiltinScalarFnVariant::Sync(Arc::new( self.variants[0].as_ref().clone(), ))); } ```
1 parent 0ced720 commit 038c058

12 files changed

Lines changed: 32 additions & 1276 deletions

File tree

examples/hello/hello/__init__.py

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,6 @@ def greet(name: Expression) -> Expression:
1313
return daft.get_function("greet", name)
1414

1515

16-
def byte_length(input: Expression) -> Expression:
17-
"""Return the byte length of a string or binary column."""
18-
return daft.get_function("byte_length", input)
19-
20-
2116
def string_count(name: Expression) -> Expression:
2217
"""Count non-null strings."""
2318
return daft.get_aggregate_function("string_count", name)

examples/hello/src/lib.rs

Lines changed: 15 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::{ffi::CStr, sync::Arc};
22

3-
use arrow_array::{Array, ArrayRef};
3+
use arrow_array::{Array, ArrayRef, builder::StringBuilder, cast::AsArray};
44
use arrow_schema::{DataType, Field};
55
use daft_ext::prelude::*;
66

@@ -12,31 +12,24 @@ struct HelloExtension;
1212
impl DaftExtension for HelloExtension {
1313
fn install(session: &mut dyn DaftSession) {
1414
session.define_function(Arc::new(Greet));
15-
session.define_function(Arc::new(ByteLengthStr));
16-
session.define_function(Arc::new(ByteLengthBin));
1715
session.define_aggregate_function(Arc::new(StringCount));
1816
}
1917
}
2018

21-
// ── Scalar Function (row-level macro) ──────────────────────────────
22-
23-
#[daft_func]
24-
fn greet(name: &str) -> String {
25-
format!("Hello, {}!", name)
26-
}
27-
28-
// ── Overloaded Function ────────────────────────────────────────────
29-
// Two variants registered under the same name "byte_length".
30-
// The host selects the right one at plan time based on input types.
31-
32-
#[daft_func(name = "byte_length")]
33-
fn byte_length_str(input: &str) -> i64 {
34-
input.len() as i64
35-
}
36-
37-
#[daft_func(name = "byte_length")]
38-
fn byte_length_bin(input: &[u8]) -> i64 {
39-
input.len() as i64
19+
// ── Scalar Function ────────────────────────────────────────────────
20+
21+
#[daft_func_batch(return_dtype = DataType::Utf8)]
22+
fn greet(input: ArrayRef) -> DaftResult<ArrayRef> {
23+
let names = input.as_string::<i64>();
24+
let mut builder = StringBuilder::with_capacity(names.len(), names.len() * 16);
25+
for i in 0..names.len() {
26+
if names.is_null(i) {
27+
builder.append_null();
28+
} else {
29+
builder.append_value(format!("Hello, {}!", names.value(i)));
30+
}
31+
}
32+
Ok(Arc::new(builder.finish()))
4033
}
4134

4235
// ── Aggregate Function ─────────────────────────────────────────────

examples/hello/tests/test_hello.py

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import hello
44
import pytest
5-
from hello import byte_length, greet, string_count
5+
from hello import greet, string_count
66

77
import daft
88
from daft import col
@@ -57,36 +57,6 @@ def test_greet_show(capsys):
5757
print(captured)
5858

5959

60-
def test_byte_length_string():
61-
sess = Session()
62-
sess.load_extension(hello)
63-
64-
df = daft.from_pydict({"data": ["hello", "hi", None]})
65-
66-
with sess:
67-
result = df.select(byte_length(col("data"))).collect().to_pydict()
68-
69-
values = result["byte_length"]
70-
assert values[0] == 5
71-
assert values[1] == 2
72-
assert values[2] is None
73-
74-
75-
def test_byte_length_binary():
76-
sess = Session()
77-
sess.load_extension(hello)
78-
79-
df = daft.from_pydict({"data": [b"\x00\x01\x02", b"\xff", None]})
80-
81-
with sess:
82-
result = df.select(byte_length(col("data"))).collect().to_pydict()
83-
84-
values = result["byte_length"]
85-
assert values[0] == 3
86-
assert values[1] == 1
87-
assert values[2] is None
88-
89-
9060
def test_string_count():
9161
sess = Session()
9262
sess.load_extension(hello)

src/daft-catalog/src/bindings.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,6 @@ impl<T> Bindings<T> {
9090
.unwrap_or_default()
9191
}
9292

93-
/// Mutable lookup by exact name.
94-
pub fn get_mut(&mut self, name: &str) -> Option<&mut T> {
95-
self.bindings.get_mut(name)
96-
}
97-
9893
/// Removes the binding if it exists (exact-case).
9994
pub fn remove(&mut self, name: &str) {
10095
self.bindings.remove(name);

src/daft-dsl/src/functions/scalar.rs

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ impl From<BuiltinScalarFn> for ExprRef {
176176
/// the ability for *type* resolution during planning via get_function. We can
177177
/// build rule-based type resolution at a later time.
178178
///
179-
pub trait ScalarFunctionFactory: Send + Sync + 'static {
179+
pub trait ScalarFunctionFactory: Send + Sync {
180180
/// The name of this function.
181181
fn name(&self) -> &'static str;
182182

@@ -198,12 +198,6 @@ pub trait ScalarFunctionFactory: Send + Sync + 'static {
198198
args: FunctionArgs<ExprRef>,
199199
schema: &Schema,
200200
) -> DaftResult<BuiltinScalarFnVariant>;
201-
202-
/// Enables downcasting to concrete types for overload resolution.
203-
fn as_any(&self) -> &dyn std::any::Any;
204-
205-
/// Mutable downcasting for overload accumulation.
206-
fn as_any_mut(&mut self) -> &mut dyn std::any::Any;
207201
}
208202

209203
/// This is a concrete implementation of a ScalarFunction.
@@ -365,12 +359,4 @@ impl ScalarFunctionFactory for DynamicScalarFunction {
365359
) -> DaftResult<BuiltinScalarFnVariant> {
366360
Ok(self.0.clone())
367361
}
368-
369-
fn as_any(&self) -> &dyn std::any::Any {
370-
self
371-
}
372-
373-
fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
374-
self
375-
}
376362
}

src/daft-ext-internal/src/function.rs

Lines changed: 0 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -248,14 +248,6 @@ impl ScalarFunctionFactory for ScalarFunctionHandle {
248248
) -> DaftResult<BuiltinScalarFnVariant> {
249249
Ok(BuiltinScalarFnVariant::Sync(Arc::new(self.clone())))
250250
}
251-
252-
fn as_any(&self) -> &dyn std::any::Any {
253-
self
254-
}
255-
256-
fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
257-
self
258-
}
259251
}
260252

261253
/// Create a [`ScalarFunctionFactory`] from an `FFI_ScalarFunction` vtable.
@@ -269,74 +261,6 @@ pub fn into_scalar_function_factory(
269261
Arc::new(ScalarFunctionHandle::new(ffi, module))
270262
}
271263

272-
/// Create a concrete [`ScalarFunctionHandle`] from an `FFI_ScalarFunction` vtable.
273-
///
274-
/// Unlike [`into_scalar_function_factory`], this returns the concrete type so callers
275-
/// can build [`OverloadedScalarFunctionFactory`] from multiple handles.
276-
pub fn into_scalar_function_handle(
277-
ffi: FFI_ScalarFunction,
278-
module: Arc<ModuleHandle>,
279-
) -> Arc<ScalarFunctionHandle> {
280-
Arc::new(ScalarFunctionHandle::new(ffi, module))
281-
}
282-
283-
/// Overloaded function factory holding multiple type-signature variants.
284-
///
285-
/// At plan time, probes each variant's `get_return_field` with the actual input
286-
/// types — the first variant whose arguments are an exact match is selected.
287-
/// No implicit coercions are attempted.
288-
#[derive(Clone)]
289-
pub struct OverloadedScalarFunctionFactory {
290-
name: &'static str,
291-
variants: Vec<Arc<ScalarFunctionHandle>>,
292-
}
293-
294-
impl OverloadedScalarFunctionFactory {
295-
pub fn new(name: &'static str, first: Arc<ScalarFunctionHandle>) -> Self {
296-
Self {
297-
name,
298-
variants: vec![first],
299-
}
300-
}
301-
302-
pub fn add_variant(&mut self, variant: Arc<ScalarFunctionHandle>) {
303-
self.variants.push(variant);
304-
}
305-
}
306-
307-
impl ScalarFunctionFactory for OverloadedScalarFunctionFactory {
308-
fn name(&self) -> &'static str {
309-
self.name
310-
}
311-
312-
fn get_function(
313-
&self,
314-
args: FunctionArgs<ExprRef>,
315-
schema: &Schema,
316-
) -> DaftResult<BuiltinScalarFnVariant> {
317-
for variant in &self.variants {
318-
if variant.get_return_field(args.clone(), schema).is_ok() {
319-
return Ok(BuiltinScalarFnVariant::Sync(Arc::new(
320-
variant.as_ref().clone(),
321-
)));
322-
}
323-
}
324-
Err(DaftError::TypeError(format!(
325-
"no matching overload for function '{}' ({} variant(s) available)",
326-
self.name,
327-
self.variants.len(),
328-
)))
329-
}
330-
331-
fn as_any(&self) -> &dyn std::any::Any {
332-
self
333-
}
334-
335-
fn as_any_mut(&mut self) -> &mut dyn std::any::Any {
336-
self
337-
}
338-
}
339-
340264
#[cfg(test)]
341265
mod tests {
342266
use std::ffi::{CString, c_int, c_void};

0 commit comments

Comments
 (0)