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.

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

Deprecate ScalarUDF::invoke and invoke_no_args for invoke_batch #13179

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 30, 2024

invoke_batch is the one used now. The others are no longer in use and we should deprecate and remove them.

Extracted from #13174

`invoke_batch` is the one used now. The others are no longer in use and
we should deprecate and remove them.
@github-actions github-actions bot added logical-expr Logical plan and expressions functions labels Oct 30, 2024
@@ -96,6 +96,7 @@ fn criterion_benchmark(c: &mut Criterion) {

b.iter(|| {
black_box(
#[allow(deprecated)] // TODO use invoke_batch
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do #[warn(deprecated)] instead? My vision it should output the note in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

i though we treat warnings as errors. will this work?

@comphead
Copy link
Contributor

Like that

#[warn(deprecated)]

#[deprecated(note = "Please use foo")]
fn m1() {
    
}

fn main() {
    m1();
    println!("Hello, world!");
}

warning: use of deprecated function `m1`: Please use foo
 --> src/main.rs:9:5
  |
9 |     m1();
  |     ^^
  |
  = note: `#[warn(deprecated)]` on by default

This will make the code cleaner

@findepi
Copy link
Member Author

findepi commented Oct 30, 2024

@comphead thanks for your suggestion. i tried this. I changed one of the suppressions to #[warn(deprecated)] and got failue locally

dev/rust_lint.sh
+ cargo fmt --all -- --check
+ cd datafusion-cli
+ cargo fmt --all -- --check
+ cargo clippy --all-targets --workspace --features avro,pyarrow -- -D warnings
   Compiling pyo3-build-config v0.22.5
   Compiling pyo3-macros-backend v0.22.5
   Compiling pyo3-ffi v0.22.5
   Compiling pyo3 v0.22.5
   Compiling pyo3-macros v0.22.5
    Checking arrow v53.2.0
    Checking datafusion-common v42.1.0 (/Users/findepi/repos/datafusion/datafusion/common)
    Checking datafusion-expr-common v42.1.0 (/Users/findepi/repos/datafusion/datafusion/expr-common)
    Checking test-utils v0.1.0 (/Users/findepi/repos/datafusion/test-utils)
    Checking datafusion-proto-common v42.1.0 (/Users/findepi/repos/datafusion/datafusion/proto-common)
    Checking datafusion-physical-expr-common v42.1.0 (/Users/findepi/repos/datafusion/datafusion/physical-expr-common)
    Checking datafusion-functions-aggregate-common v42.1.0 (/Users/findepi/repos/datafusion/datafusion/functions-aggregate-common)
    Checking datafusion-functions-window-common v42.1.0 (/Users/findepi/repos/datafusion/datafusion/functions-window-common)
    Checking datafusion-expr v42.1.0 (/Users/findepi/repos/datafusion/datafusion/expr)
    Checking datafusion-execution v42.1.0 (/Users/findepi/repos/datafusion/datafusion/execution)
    Checking datafusion-physical-expr v42.1.0 (/Users/findepi/repos/datafusion/datafusion/physical-expr)
    Checking datafusion-sql v42.1.0 (/Users/findepi/repos/datafusion/datafusion/sql)
    Checking datafusion-functions v42.1.0 (/Users/findepi/repos/datafusion/datafusion/functions)
    Checking datafusion-functions-aggregate v42.1.0 (/Users/findepi/repos/datafusion/datafusion/functions-aggregate)
    Checking datafusion-physical-plan v42.1.0 (/Users/findepi/repos/datafusion/datafusion/physical-plan)
    Checking datafusion-functions-window v42.1.0 (/Users/findepi/repos/datafusion/datafusion/functions-window)
    Checking datafusion-optimizer v42.1.0 (/Users/findepi/repos/datafusion/datafusion/optimizer)
    Checking datafusion-functions-nested v42.1.0 (/Users/findepi/repos/datafusion/datafusion/functions-nested)
error: use of deprecated method `datafusion_expr::ScalarUDF::invoke`: Use `invoke_batch` instead
  --> datafusion/functions/benches/date_bin.rs:50:21
   |
50 |                 udf.invoke(&[interval.clone(), timestamps.clone()])
   |                     ^^^^^^
   |
   = note: `-D deprecated` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(deprecated)]`

error: could not compile `datafusion-functions` (bench "date_bin") due to 1 previous error
warning: build failed, waiting for other jobs to finish...

it looks like these lines are crucial

   = note: `-D deprecated` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(deprecated)]`

@comphead
Copy link
Contributor

oh bummer -D denies warning, let me check real quick if we can introduce any exclusions

@findepi
Copy link
Member Author

findepi commented Oct 30, 2024

@comphead thanks
also, i hope this is temporary. we will update all these places no later than we remove these deprecated functions, and hopefully sooner.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @findepi

@comphead
Copy link
Contributor

@comphead thanks also, i hope this is temporary. we will update all these places no later than we remove these deprecated functions, and hopefully sooner.

Lets go with PR and later I can play with exclusions

@alamb alamb merged commit b7f4db4 into apache:main Nov 1, 2024
25 checks passed
@alamb
Copy link
Contributor

alamb commented Nov 1, 2024

Thanks @comphead and @findepi

@findepi findepi deleted the findepi/deprecate-scalarudf-invoke-and-invoke-no-args-for-invoke-batch-7eada1 branch November 1, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants