Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-3146636245
Closing as #16970 is preferred approach
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go
Omega359 closed pull request #13527: feat: Add ConfigOptions to
ScalarFunctionArgs
URL: https://github.com/apache/datafusion/pull/13527
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific c
Omega359 commented on PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#issuecomment-3145870578
> Good:
>
> It allows function behavior to be dynamically adapted based on the
execution context (e.g., user locale, timezone, session settings, environment
specifics). Espec
Omega359 commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2248021021
##
datafusion/core/src/execution/session_state.rs:
##
@@ -738,6 +738,17 @@ impl SessionState {
self.config.options()
}
+/// return the configu
Omega359 commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2248022415
##
datafusion/core/src/execution/session_state.rs:
##
@@ -1887,8 +1898,8 @@ impl OptimizerConfig for SessionState {
&self.execution_props.alias_generator
Omega359 commented on PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#issuecomment-3141903910
> I have a few API suggestions. I sketched the major one out here
>
> * [Omega359#6](https://github.com/Omega359/arrow-datafusion/pull/6)
>
>
> If we go with
alamb commented on PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#issuecomment-3141380597
FYI @findepi (who I believe is away this week but may be interested in this
PR)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on
alamb commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2246374299
##
datafusion/core/src/execution/session_state.rs:
##
@@ -738,6 +738,17 @@ impl SessionState {
self.config.options()
}
+/// return the configurat
Omega359 commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2245815418
##
datafusion/core/src/execution/session_state.rs:
##
@@ -738,6 +738,17 @@ impl SessionState {
self.config.options()
}
+/// return the configu
comphead commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2245812112
##
datafusion/optimizer/src/optimizer.rs:
##
@@ -137,13 +137,15 @@ impl OptimizerContext {
Self {
query_execution_start_time: Utc::now(),
Omega359 commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2245809712
##
datafusion/core/src/execution/session_state.rs:
##
@@ -738,6 +738,17 @@ impl SessionState {
self.config.options()
}
+/// return the configu
comphead commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2245801709
##
datafusion/core/src/execution/session_state.rs:
##
@@ -738,6 +738,17 @@ impl SessionState {
self.config.options()
}
+/// return the configu
comphead commented on code in PR #16970:
URL: https://github.com/apache/datafusion/pull/16970#discussion_r2245799840
##
datafusion/core/src/execution/session_state.rs:
##
@@ -738,6 +738,17 @@ impl SessionState {
self.config.options()
}
+/// return the configu
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-3001594979
The async udf approach using an ExecutionPlan is interesting. I think it's
an approach that could work for scalar udfs as well but it would take some time
to impl
--
This is an a
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2997941934
I noticed that in the API but I haven't yet had a chance to see how it's
implemented. It would be awesome if it's compatible/workable with non-async
functions too
--
This is an a
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2997826090
Interestingly I noticed that @goldmedal added `ConfigOptions` to the `async
UDF` API in https://github.com/apache/datafusion/pull/14837
Maybe we can do something similar with no
github-actions[bot] commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2986358525
Thank you for your contribution. Unfortunately, this pull request is stale
because it has been open 60 days with no activity. Please remove the stale
label or comment or
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2812860089
> BTW how many fields from ConfigOptions really need to be copied? Can we
just add the ones you need for spark? Or do we need a huge pile of them?
For starters I was thinking
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2810864566
BTW how many fields from ConfigOptions really need to be copied? Can we just
add the ones you need for spark? Or do we need a huge pile of them?
--
This is an automated message from
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2810863393
> OptimizerConfig - this one is strange - it copies some things from
ExecutionProps instead of just using it directly. Either this weirdness needs
to be expanded or OptimizerConfig nee
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2800015976
I've spent some time looking at using ExecutionProps for this and while I
think it'll work it's still a lot of churn. That churn is largely because of
two reasons:
1. We woul
alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1997561888
##
datafusion/common/src/config.rs:
##
@@ -777,7 +778,20 @@ impl ConfigField for ConfigOptions {
}
}
+static CONFIG_OPTIONS_SINGLETON: LazyLock> =
+LazyL
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2794063751
> Another idea:
>
> Since
[`ExecutionProps`](https://docs.rs/datafusion/latest/datafusion/execution/context/struct.ExecutionProps.html)
is already threaded all the way throug
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2792307005
Another idea:
Since
[`ExecutionProps`](https://docs.rs/datafusion/latest/datafusion/execution/context/struct.ExecutionProps.html)
is already threaded all the way through and is
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2789803674
Not sure I'd call a clone or two overhead but I agree with you on the
plumbing. It's a lot.
In JVM land I would likely have used a thread local variable to hold a
session id
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2789191368
I keep thinking about this PR in the back of my head. The idea of being able
to customize functions based on configuration options makes total sense to me,
but having to plumb it down
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2727338730
@Omega359 -- I started reviewing this PR today. I am still getting my head
around what the API change is / what this would mean for downstream users. I'll
try and spend more time over
alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1994240472
##
datafusion/core/Cargo.toml:
##
@@ -144,7 +144,7 @@ zstd = { version = "0.13", optional = true,
default-features = false }
[dev-dependencies]
async-trait = {
Omega359 commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1993977680
##
datafusion/core/Cargo.toml:
##
@@ -144,7 +144,7 @@ zstd = { version = "0.13", optional = true,
default-features = false }
[dev-dependencies]
async-trait =
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2603326190
Converting to draft as this PR is accumulating conflicts and it seems like
it is not ready for merge yet
--
This is an automated message from the Apache Git Service.
To respond to th
alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1899467353
##
datafusion/physical-expr/src/scalar_function.rs:
##
@@ -243,6 +292,7 @@ pub fn create_physical_expr(
Arc::new(fun.clone()),
input_phy_ex
Omega359 commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1899197257
##
datafusion/physical-expr/src/scalar_function.rs:
##
@@ -243,6 +292,7 @@ pub fn create_physical_expr(
Arc::new(fun.clone()),
input_phy
alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1899131349
##
datafusion-examples/examples/composed_extension_codec.rs:
##
@@ -71,8 +71,10 @@ async fn main() {
// deserialize proto back to execution plan
let runt
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2564723081
> Examples check failure is transient I believe.
I restarted the checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2564560762
Examples check failure is transient I believe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2560262193
I think this may be ready for review again. For this round I refactored the
code to use &ConfigOptions everywhere except for ScalarFunctionExpr so the cost
for cloning ConfigOptions
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2506774637
> > Maybe we could change the semantics so that `SessionConfig` has a
`Arc` which was cloned when it was modified
(`Arc::unwrap_or_clone()` style) 🤔
>
> Certainly possible, I
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2504633485
Marking as draft as I think this PR is no longer waiting on feedback. Please
mark it as ready for review when it is ready for another look
--
This is an automated message from the A
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2499032455
Yeah, it is a tricky one for sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the sp
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2498200913
> This PR seems to require `config_options` to be cloned many times now. I
wonder if it is possible to avoid that 🤔. I took a brief look and it seems to
be somewhat challenging as S
alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1856449804
##
datafusion-examples/examples/expr_api.rs:
##
@@ -356,8 +357,13 @@ fn type_coercion_demo() -> Result<()> {
// Evaluation with an expression that has not bee
alamb commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2496304460
I plan to review this carefully tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to
Omega359 commented on PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#issuecomment-2494065223
There is a lot of file changes here but most of the important changes are in
scalar_function.rs, There is a todo in
[expr_simplifier.rs](https://github.com/apache/datafusion/pull/13
43 matches
Mail list logo
