jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2667238494
Close by #14440
--
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 comme
jayzhan211 closed pull request #14268: Fix Type Coercion for UDF Arguments
URL: https://github.com/apache/datafusion/pull/14268
--
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 comment.
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1944712928
##
datafusion/expr-common/src/signature.rs:
##
@@ -376,7 +383,9 @@ impl TypeSignature {
TypeSignature::Coercible(types) => types
.
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1944643137
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1944643767
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1944640302
##
datafusion/expr-common/src/signature.rs:
##
@@ -213,9 +221,8 @@ pub enum TypeSignatureClass {
Interval,
Duration,
Native(LogicalTypeRef),
-
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1944122850
##
datafusion/functions/src/string/ascii.rs:
##
@@ -93,6 +95,33 @@ impl ScalarUDFImpl for AsciiFunc {
make_scalar_function(ascii, vec![])(args)
}
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942524406
##
datafusion/functions/src/string/ascii.rs:
##
@@ -93,6 +95,33 @@ impl ScalarUDFImpl for AsciiFunc {
make_scalar_function(ascii, vec![])(args)
}
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942560239
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942559050
##
datafusion/functions/src/string/ascii.rs:
##
@@ -93,6 +95,33 @@ impl ScalarUDFImpl for AsciiFunc {
make_scalar_function(ascii, vec![])(args)
}
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942537530
##
datafusion/functions/src/string/ascii.rs:
##
@@ -93,6 +95,33 @@ impl ScalarUDFImpl for AsciiFunc {
make_scalar_function(ascii, vec![])(args)
}
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942541549
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942535860
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942535860
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942530489
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +541,36 @@ fn get_valid_types(
match target_type_class {
jayzhan-synnada commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942523715
##
datafusion/functions/src/string/ascii.rs:
##
@@ -93,6 +95,33 @@ impl ScalarUDFImpl for AsciiFunc {
make_scalar_function(ascii, vec![])(args)
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942525348
##
datafusion/functions/src/string/bit_length.rs:
##
@@ -106,6 +108,33 @@ impl ScalarUDFImpl for BitLengthFunc {
}
}
+fn coerce_types(&self,
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942524406
##
datafusion/functions/src/string/ascii.rs:
##
@@ -93,6 +95,33 @@ impl ScalarUDFImpl for AsciiFunc {
make_scalar_function(ascii, vec![])(args)
}
findepi commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2635850123
> I fully recognize this creates behavior that diverges from
PostgreSQL/DuckDB semantics for the various UDFs in this PR. However, there’s a
critical distinction: **System contracts
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942056824
##
datafusion/expr-common/src/signature.rs:
##
@@ -213,9 +221,8 @@ pub enum TypeSignatureClass {
Interval,
Duration,
Native(LogicalTypeRef),
-
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942056824
##
datafusion/expr-common/src/signature.rs:
##
@@ -213,9 +221,8 @@ pub enum TypeSignatureClass {
Interval,
Duration,
Native(LogicalTypeRef),
-
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1942056824
##
datafusion/expr-common/src/signature.rs:
##
@@ -213,9 +221,8 @@ pub enum TypeSignatureClass {
Interval,
Duration,
Native(LogicalTypeRef),
-
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2635031823
> @jayzhan211 To keep it simple ill just remove AnyNative and use
coerce_types so we don't block this PR any longer. We can have a larger
discussion and align on goals afterwards
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2634598214
@jayzhan211 To keep it simple ill just remove AnyNative and use coerce_types
so we don't block this PR any longer. We can have a larger discussion and align
on goals afterwards!
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2633097400
> This is the point, I don't quite understand why adding signature for
`default_cast_to` logic helps. I thought you want to solve the issues for these
datafusion functions, so I
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2633007543
Btw, if your proposed signature doesn't help for any functions in
datafusion. You should use `TypeSignature::UserDefined` it is used for any
other customize logic for downstream.
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2633000378
> if it helps downstream projects
This is the point, I don't quite understand why adding signature for
`default_cast_to` logic helps. I thought you want to solve the issues
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2632975463
> As we have discussed, we should avoid using old Coercible signature and
also the TypeSignatureClass that is used in Coercible, because any change might
impact downstream projec
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2632952904
> @jayzhan211 What did I miss here?
As we have discussed, we should avoid using old Coercible signature and also
the TypeSignatureClass that is used in Coercible. Therefore
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2632940583
> > I may add a new TypeSignatureClass in this pr to expose default_cast_for
so that it's accessible by downstream users
>
> Not sure do we need this 🤔 since we might not n
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2632938494
> > @shehabgamin #14440 I come out a flexible version of
Signature::CoercibleV2 (temporary name), it can replace `Signature::Coercible`,
`Signature::String`, `Signature::Numeric`,
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2632894156
> @shehabgamin #14440 I come out a flexible version of
Signature::CoercibleV2 (temporary name), it can replace `Signature::Coercible`,
`Signature::String`, `Signature::Numeric`,
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2632259185
I'll work on this tonight @alamb @jayzhan211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL abo
alamb commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2631352326
FWIW I think @Omega359 hit the same "int no longer automatically coerces to
String" in his application too:
https://github.com/apache/datafusion/issues/14230#issuecomment-2631350057
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2631043032
@shehabgamin #14440 I come out a flexible version of Signature::CoercibleV2
(temporary name), it can be replace `Signature::String`, `Signature::Numeric`,
`Signature::Exact`, `Sig
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2629300546
> I'll get to this sometime in the next few hours. Taking a detour to review
and comment on https://github.com/apache/datafusion/pull/14392.
@jayzhan211 @alamb, I bit off m
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2629247219
> > If the approach looks good to you too, we can go ahead!
>
> Sounds good! This works for now since we'll be working towards a better
design in the future.
>
> I ma
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2629245515
> If the approach looks good to you too, we can go ahead!
Sounds good! This works for now since we'll be working towards a better
design in the future.
I may add a new `
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2629240637
> ### Topic 1
> > @findepi I had thought about using Signature:: User-defined everywhere
before but was rejected for some reason. As the discussion here, I think we
need this t
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2629239585
### Topic 1
> @findepi I had thought about using Signature:: User-defined everywhere
before but was rejected for some reason. As the discussion here, I think we
need this to m
alamb commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2629035590
Given I feel like we have not yet reached consensus on this issue I made a
release branch for 45 without this change
- https://github.com/apache/datafusion/issues/14008#issuecomment-
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628939208
> @shehabgamin can we try to use Singatur::User-defined coerce types for
those functions, the change is relatively trivial and the impact is smaller
than the current one
@f
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628936121
@shehabgamin can we try to use coerce types for those functions, the change
is relatively trivial and less breakage compare to the current one
--
This is an automated message fr
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628913239
> > So what is the actual regressions to be resolved? I don't remember
signature coericible is consistent with the docs before so it is not a
regression to be fixed now. If suppor
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628900809
> So what is the actual regressions to be resolved? I don't remember
signature coericible is consistent with the docs before so it is not a
regression to be fixed now. If support
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628591052
> > > I don't have a strong opinion about how exactly the coercion rules
should be changed.
> > >
> > >
> > >
> > > It does seem to me that we keep churning / thras
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628583804
> > I don't have a strong opinion about how exactly the coercion rules
should be changed.
> >
> >
> >
> > It does seem to me that we keep churning / thrashing on co
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2628446082
> I don't have a strong opinion about how exactly the coercion rules should
be changed.
>
>
>
> It does seem to me that we keep churning / thrashing on coercion (a
alamb commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2627866762
I don't have a strong opinion about how exactly the coercion rules should be
changed.
It does seem to me that we keep churning / thrashing on coercion (and
introducing regressi
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2626728678
> First of all, we need to have a out of box behavior for each function, and
at the same time easy to be customizable. The Signature Coercible now becomes
problematic, if we foll
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2626205413
> > > @jayzhan211 and @shehabgamin what is the status of this PR? It looks
to me like there are some unresolved comments
> > > It looks like there are some unresolved comments
shehabgamin commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2626079378
> > @jayzhan211 and @shehabgamin what is the status of this PR? It looks to
me like there are some unresolved comments
> > It looks like there are some unresolved comments lik
jayzhan211 commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2626045546
> @jayzhan211 and @shehabgamin what is the status of this PR? It looks to
me like there are some unresolved comments
>
> It looks like there are some unresolved comments l
alamb commented on PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#issuecomment-2625430773
@jayzhan211 and @shehabgamin what is the status of this PR? It looks to me
like there are some unresolved comments
It looks like there are some unresolved comments like
https:
alamb commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1936187986
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -571,8 +601,10 @@ select repeat('-1.2', arrow_cast(3, 'Int32'));
-1.2-1.2-1.2
-query error DataFusion
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933110734
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
T
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933110734
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
T
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933110734
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
T
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933063432
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
T
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933056565
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933055108
##
datafusion/functions/src/string/contains.rs:
##
@@ -59,7 +64,13 @@ impl Default for ContainsFunc {
impl ContainsFunc {
pub fn new() -> Self {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933060341
##
datafusion/functions/src/string/bit_length.rs:
##
@@ -55,7 +58,10 @@ impl Default for BitLengthFunc {
impl BitLengthFunc {
pub fn new() -> Self {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933056565
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933055108
##
datafusion/functions/src/string/contains.rs:
##
@@ -59,7 +64,13 @@ impl Default for ContainsFunc {
impl ContainsFunc {
pub fn new() -> Self {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933055108
##
datafusion/functions/src/string/contains.rs:
##
@@ -59,7 +64,13 @@ impl Default for ContainsFunc {
impl ContainsFunc {
pub fn new() -> Self {
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933052292
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
T
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933048065
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
T
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933034910
##
datafusion/functions/src/string/contains.rs:
##
@@ -59,7 +64,13 @@ impl Default for ContainsFunc {
impl ContainsFunc {
pub fn new() -> Self {
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933031384
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -571,8 +601,10 @@ select repeat('-1.2', arrow_cast(3, 'Int32'));
-1.2-1.2-1.2
-query error DataFu
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1933030522
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -344,6 +344,16 @@ SELECT ascii('💯a')
128175
+query I
+SELECT ascii('222')
+
+50
+
+query I
+S
alamb commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1932866009
##
datafusion/functions/src/string/bit_length.rs:
##
@@ -55,7 +58,10 @@ impl Default for BitLengthFunc {
impl BitLengthFunc {
pub fn new() -> Self {
S
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929718209
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -344,6 +344,16 @@ SELECT ascii('💯a')
128175
+query I
+SELECT ascii('222')
+
+50
+
+query I
+
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929718209
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -344,6 +344,16 @@ SELECT ascii('💯a')
128175
+query I
+SELECT ascii('222')
+
+50
+
+query I
+
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929723820
##
datafusion/expr/src/type_coercion/functions.rs:
##
@@ -584,23 +544,7 @@ fn get_valid_types(
match target_type_class {
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929721662
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -571,8 +601,10 @@ select repeat('-1.2', arrow_cast(3, 'Int32'));
-1.2-1.2-1.2
-query error DataF
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929718209
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -344,6 +344,16 @@ SELECT ascii('💯a')
128175
+query I
+SELECT ascii('222')
+
+50
+
+query I
+
jayzhan211 commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929710139
##
datafusion/sqllogictest/test_files/expr.slt:
##
@@ -344,6 +344,16 @@ SELECT ascii('💯a')
128175
+query I
+SELECT ascii('222')
+
+50
+
+query I
+S
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929701613
##
datafusion/optimizer/src/analyzer/type_coercion.rs:
##
@@ -2133,4 +2133,77 @@ mod test {
assert_analyzed_plan_eq(Arc::new(TypeCoercion::new()), pla
shehabgamin commented on code in PR #14268:
URL: https://github.com/apache/datafusion/pull/14268#discussion_r1929529897
##
datafusion/optimizer/src/analyzer/type_coercion.rs:
##
@@ -2133,4 +2133,77 @@ mod test {
assert_analyzed_plan_eq(Arc::new(TypeCoercion::new()), pla
79 matches
Mail list logo
