Skip to content

Minor: fix signature fn octect_length() - #10726

Merged
alamb merged 2 commits into
apache:mainfrom
marvinlanhenke:fix_fn_octet_length
May 30, 2024
Merged

Minor: fix signature fn octect_length()#10726
alamb merged 2 commits into
apache:mainfrom
marvinlanhenke:fix_fn_octet_length

Conversation

@marvinlanhenke

@marvinlanhenkemarvinlanhenke commented May 30, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#10676.

Rationale for this change

  • fix a minor issue about consistency
  • fn signature should be the same as fn bit_length()

What changes are included in this PR?

  • changed octet_length to accept Expr instead of Vec<Expr>

Are these changes tested?

Yes. Existing tests.

Are there any user-facing changes?

No.

alamb
alamb approved these changes May 30, 2024
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me 🙏 thank ou @marvinlanhenke

Would it be possible to add a test here so we don't break it again:

datafusion/datafusion/core/tests/expr_api/mod.rs

Lines 51 to 63 in 904f0db

evaluate_expr_test(
col("id").eq(lit(2i32)),
vec![
"+-------+",
"| expr |",
"+-------+",
"| false |",
"| true |",
"| false |",
"+-------+",
],
);
}
?

(run via cargo run --test core_integration)

@github-actionsgithub-actionsBot added the core Core DataFusion crate label May 30, 2024
@marvinlanhenke

marvinlanhenke commented May 30, 2024

Copy link
Copy Markdown
ContributorAuthor

thanks for the quick response @alamb - I added the test

comphead
comphead reviewed May 30, 2024
View reviewed changes
Comment threaddatafusion/core/tests/expr_api/mod.rs
comphead
comphead reviewed May 30, 2024
View reviewed changes
Comment threaddatafusion/functions/src/string/mod.rs
alamb
alamb approved these changes May 30, 2024
View reviewed changes
Comment threaddatafusion/core/tests/expr_api/mod.rs
@alamb

alamb commented May 30, 2024

Copy link
Copy Markdown
Contributor

Thanks again @marvinlanhenke and @comphead

@alamb
alamb merged commit 100b30e into apache:mainMay 30, 2024
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jun 13, 2024
@Michael-J-Ward
fix signature for octet_length
656905c
Ref apache/datafusion#10726
andygrove pushed a commit to apache/datafusion-python that referenced this pull request Jun 14, 2024
@Michael-J-Ward
Upgrade datafusion 39 (#728)
b5446ef
* deps: update datafusion to 39.0.0, pyo3 to 0.21, and object_store to 0.10.1
`datafusion-common` also depends on `pyo3`, so they need to be upgraded together.
* feat: remove GetIndexField
datafusion replaced Expr::GetIndexField with a FieldAccessor trait.
Ref apache/datafusion#10568
Ref apache/datafusion#10769
* feat: update ScalarFunction
The field `func_name` was changed to `func` as part of removing `ScalarFunctionDefinition` upstream.
Ref apache/datafusion#10325
* feat: incorporate upstream array_slice fixes
Fixes#670
* update ExectionPlan::children impl for DatasetExec
Ref apache/datafusion#10543
* update value_interval_daytime
Ref apache/arrow-rs#5769
* update regexp_replace and regexp_match
Fixes#677
* add gil-refs feature to pyo3
This silences pyo3's deprecation warnings for its new Bounds api.
It's the 1st step of the migration, and should be removed before merge.
Ref https://pyo3.rs/v0.21.0/migration#from-020-to-021
* fix signature for octet_length
Ref apache/datafusion#10726
* update signature for covar_samp
AggregateUDF expressions now have a builder API design, which removes arguments like filter and order_by
Ref apache/datafusion#10545
Ref apache/datafusion#10492
* convert covar_pop to expr_fn api
Ref: https://github.com/apache/datafusion/pull/10418/files
* convert median to expr_fn api
Ref apache/datafusion#10644
* convert variance sample to UDF
Ref apache/datafusion#10667
* convert first_value and last_value to UDFs
Ref apache/datafusion#10648
* checkpointing with a few todos to fix remaining compile errors
* impl PyExpr::python_value for IntervalDayTime and IntervalMonthDayNano
* convert sum aggregate function to UDF
* remove unnecessary clone on double reference
* apply cargo fmt
* remove duplicate allow-dead-code annotation
* update tpch examples for new pyarrow interval
Fixes#665
* marked q11 tpch example as expected fail
Ref #730
* add default stride of None back to array_slice
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
@marvinlanhenke@findepi
Minor: fix signaturefn octect_length()(apache#10726)
1a7efb9
* fix: signature fn octect_length
* chore: add test
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compheadcompheadcomphead left review comments
@alambalambalamb approved these changes

Assignees

No one assigned

Labels

coreCore DataFusion crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Should the arg of octet_length() be a bare Expr instead of Vec<Expr>?

3 participants

@marvinlanhenke@alamb@comphead