Re: COALESCE returns maximum of available CHAR capacity.

[stanilovsky evgeny]

I also check sqlite and mssql 2017 and obtain the same results, but i also  
found that standard has differ treatment:


9.3  Set operation result data types

 Function

 Specify the Syntax Rules and result data types for s and s having set operators.

... skip ...
  i) If any of the data types in DTS is variable-length char-
 acter string, then the result data type is variable-length
 character string with maximum length in characters equal
 to the maximum of the lengths in characters and maximum
 lengths in characters of the data types in DTS.

 ii) Otherwise, the result data type is fixed-length character
 string with length in characters equal to the maximum of
 the lengths in characters of the data types in DTS.

But i suppose that exact sizing (i.e. 'a' instead of 'a ') is more usable,  
just imagine if someone calls :

1. Form COALESCE from table1.
2. INSERT INTO SELECT Syntax with COALESCE into table2
3. Further search in table1 using table2 predicates.
'a ' not belong to table1 and cant be found, i think this is not a single  
case.


community ?


Julian, thanks !

mysql differs here:
mysql> SELECT LENGTH(a) from (SELECT COALESCE('a', '') as a) as t;
+---+
| LENGTH(a) |
+---+
| 1 |
+---+

mysql> SELECT LENGTH(a) from (SELECT COALESCE('aa', '') as a) as t;
+---+
| LENGTH(a) |
+---+
| 2 |
+---+

I don`t claim that calcite need to be the same, i just want to  
understand what is correct.
My point of view - if COALESCE transforms into CASE-WHEN statement and  
returns first not null it strange instead of expected 'a' gets 'a '.

I`m wrong ?

The current behavior looks ok to me. The expression has type CHAR(2)  
because the arguments have types CHAR(1) and CHAR(2). So ‘a’ is widened  
to ‘a ‘.


Julian

On Sep 22, 2021, at 12:00 AM, stanilovsky evgeny  
 wrote:


sorry for typo, 'fill the ticket' of course ))


hi community !
I found that COALESCE('a', 'bb') will return 'a ' <-- whitespace is  
present, i found that now it expands for maximum presented CHAR  
(RelDataType#inferReturnType logic)

sql 92 standard tolds:

'''
COALESCE (V1, V2) is equivalent to the following :
CASE WHEN V1 IS NOT NULL THEN V1 ELSE V2 END
'''

So i suppose that existing behavior is erroneous, if it`s ok i fell  
the ticket.


wdyt ?

[jira] [Created] (CALCITE-4795) In class SqlBasicCall, make the "operands" field private.

[Julian Hyde (Jira)]

Julian Hyde created CALCITE-4795:


 Summary: In class SqlBasicCall, make the "operands" field private.
 Key: CALCITE-4795
 URL: https://issues.apache.org/jira/browse/CALCITE-4795
 Project: Calcite
  Issue Type: Bug
Reporter: Julian Hyde


In {{class SqlBasicCall}}, the {{operands}} 
[field|https://github.com/apache/calcite/blob/4bc916619fd286b2c0cc4d5c653c96a68801d74e/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java#L34]
 is a {{public}} array. This seems crazy to me – any client might be writing 
into that field at any time. I propose to make the field private.

This presents some compatibility problems, because people might be depending on 
the field. So I propose a quick deprecation and removal:
 * In release 1.28 (the next release, as I write this) the field and the 
{{public SqlNode[] getOperands()}} method will be marked deprecated. We will 
mirror into another field, {{private final List operandList = 
Arrays.asList(operands);}} We can replace all uses of the {{operands}} field in 
Calcite with uses of the new {{operandList}} field.
 * In release 1.29 (the release after next) the {{operands}} field and the 
{{getOperands()}} method will be removed. People can operate using 
{{List getOperandList()}} and {{setOperand(int, SqlNode)}} methods 
that are inherited from {{SqlCall}}.

After the field is a private list, we could consider making it an immutable 
list. The list would be copied when people call {{setOperand}}, but would not 
need to be cloned when the {{SqlBasicCall}} is created or cloned.

This case completes the work started in CALCITE-147.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Created] (CALCITE-4794) Arithmetic remainder operator should be simplified

[xzh_dz (Jira)]

xzh_dz created CALCITE-4794:
---

 Summary: Arithmetic remainder operator should be simplified
 Key: CALCITE-4794
 URL: https://issues.apache.org/jira/browse/CALCITE-4794
 Project: Calcite
  Issue Type: Improvement
Reporter: xzh_dz


When I simplify `MOD` operator in my project , an exception is thrown.

Exceptions can be reproduced as below.
{code:java}
// code placeholder
@Test void testSimplifyMod() {
  RexNode caseNode = case_(
  gt(mod(vIntNotNull(), literal(1)), literal(1)), falseLiteral,
  trueLiteral);
  checkSimplify(caseNode, "");
}
protected RexNode mod(RexNode n1, RexNode n2) {
  return rexBuilder.makeCall(SqlStdOperatorTable.MOD, n1, n2);
}
{code}
 
{code:java}

// code placeholder
Exception :

[0;31;1mFAILURE   0.3sec, org.apache.calcite.rex.RexProgramTest > 
testSimplifyMod()
java.lang.IllegalArgumentException: unbound: MOD(?0.notNullInt0, 1)
at org.apache.calcite.rex.RexInterpreter.unbound(RexInterpreter.java:92)
at 
org.apache.calcite.rex.RexInterpreter.visitCall(RexInterpreter.java:227)
at 
org.apache.calcite.rex.RexInterpreter.visitCall(RexInterpreter.java:58)
at org.apache.calcite.rex.RexCall.accept(RexCall.java:189)
at org.apache.calcite.rex.RexVisitor.visitList(RexVisitor.java:63)
at org.apache.calcite.rex.RexVisitor.visitList(RexVisitor.java:71)
at 
org.apache.calcite.rex.RexInterpreter.visitCall(RexInterpreter.java:148)
at 
org.apache.calcite.rex.RexInterpreter.visitCall(RexInterpreter.java:58)
at org.apache.calcite.rex.RexCall.accept(RexCall.java:189)
at org.apache.calcite.rex.RexVisitor.visitList(RexVisitor.java:63)
at org.apache.calcite.rex.RexVisitor.visitList(RexVisitor.java:71)
at 
org.apache.calcite.rex.RexInterpreter.visitCall(RexInterpreter.java:148)
at 
org.apache.calcite.rex.RexInterpreter.visitCall(RexInterpreter.java:58)
at org.apache.calcite.rex.RexCall.accept(RexCall.java:189)
{code}
 



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Re: SqlBasicCall.clone shallow copy

[Julian Hyde]

That looks like a bug. Cloning a SqlCall and then calling setOperand on it is 
valid, and should not modify the original SqlCall.

We tend to avoid calling ’set’ methods on SqlNodes - we treat them as immutable 
except that the validator will replace an argument with an equivalent argument 
(e.g. replacing an identifier with a fully-qualified identifier). That’s an 
explanation of why we don’t tend to hit this problem - I’m not claiming it’s 
not a bug.

Can you log a JIRA case please?

Separately, it’s crazy that the “operands" field [1] of SqlBasicCall is public. 
We should deprecate the field for one point release (mirroring it into a 
private mutable list field) and then remove it. Later we can make the private 
list field immutable (copy on write) or something.

Julian

[1] 
https://github.com/apache/calcite/blob/4bc916619fd286b2c0cc4d5c653c96a68801d74e/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java#L34
 

 




> On Sep 22, 2021, at 3:36 PM, Wen Zhang  wrote:
> 
> Hello,
> 
> I'm trying to make shallow copies of `SqlNode` objects by calling the 
> `clone(SqlParserPos pos) 
> `
>  method. However, the `SqlBasicCall` class overrides this method with an 
> implementation 
> 
>  that might return a new `SqlCall` that shares the operand array with the 
> original. This is due to it calling `SqlOperator.createCall(SqlLiteral 
> functionQualifier, SqlParserPos pos, SqlNode... operands) 
> `,
>  whose default implementation 
> 
>  creates a new `SqlBasicCall` object without copying `operands`.
> 
> Is my understanding correct?
> 
> Thank you!
> 
> Best regards,
> Wen
> 

SqlBasicCall.clone shallow copy

[Wen Zhang]

Hello,

I'm trying to make shallow copies of `SqlNode` objects by calling the 
`clone(SqlParserPos pos) 
` 
method. However, the `SqlBasicCall` class overrides this method with an 
implementation 
 
that might return a new `SqlCall` that shares the operand array with the 
original. This is due to it calling `SqlOperator.createCall(SqlLiteral 
functionQualifier, SqlParserPos pos, SqlNode... operands) 
`, 
whose default implementation 
 
creates a new `SqlBasicCall` object without copying `operands`.


Is my understanding correct?

Thank you!

Best regards,
Wen

[jira] [Created] (CALCITE-4793) CassandraAdapterDataTypesTest.testCollectionsInnerValues fails for guava <= 25.0-jre

[Alessandro Solimando (Jira)]

Alessandro Solimando created CALCITE-4793:
-

 Summary: CassandraAdapterDataTypesTest.testCollectionsInnerValues 
fails for guava <= 25.0-jre
 Key: CALCITE-4793
 URL: https://issues.apache.org/jira/browse/CALCITE-4793
 Project: Calcite
  Issue Type: Bug
  Components: cassandra-adapter
Affects Versions: 1.27.0
Reporter: Alessandro Solimando
Assignee: Alessandro Solimando


Depending on the user timezone, the test fails because the value of the 
timestamp field is not the expected one.

For instance, the following command:
{noformat}
./gradlew :cassandra:test --tests 
"org.apache.calcite.test.CassandraAdapterDataTypesTest.testCollectionsInnerValues"
 -Pguava.version=25.0-jre -Duser.timezone=GMT{noformat}
causes the following test failure:
{noformat}
java.lang.AssertionError: Expected: is "EXPR$0=1; EXPR$1=v1; 1=30; 
2=30ff87; 3=2015-05-03 11:30:54\n" but: was "EXPR$0=1; EXPR$1=v1; 1=30; 
2=30ff87; 3=2015-05-03 13:30:54\n"{noformat}
The issue is not present for guava versions >= 26.0-jre.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Created] (CALCITE-4792) SqlToRel should populate corralateId for join with corralated query in ON condition

[James Starr (Jira)]

James Starr created CALCITE-4792:


 Summary: SqlToRel should populate corralateId for join with 
corralated query in ON condition
 Key: CALCITE-4792
 URL: https://issues.apache.org/jira/browse/CALCITE-4792
 Project: Calcite
  Issue Type: Improvement
Reporter: James Starr
Assignee: James Starr






--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Created] (CALCITE-4791) RelBuilder.join incorrectly rewrites correlated conditions

[James Starr (Jira)]

James Starr created CALCITE-4791:


 Summary: RelBuilder.join incorrectly rewrites correlated conditions
 Key: CALCITE-4791
 URL: https://issues.apache.org/jira/browse/CALCITE-4791
 Project: Calcite
  Issue Type: Improvement
  Components: core
Reporter: James Starr
Assignee: James Starr


When are correlated join is rewritten to correlate + filters, if the conditions 
contains a subquery with correlate variable from the right side, the result 
rewrite is incorrect.  The correlated variable while be indexed from the 
context of the join, however in the case of right side, this should be shifted 
by the size of the left index.





--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Re: [DISCUSS] Changing from ImmutableBeans to Immutables

[Jacques Nadeau]

Just to clarify my message slightly. Rule behaviors and interfaces wouldn't
change.

The only breaking change would be RelRule would no longer expose the 'as'
function nor the EMPTY constant. So new RelRules written outside of the
Calcite project would need to change when they used either of those items.

Also, Julian pointed out that my rule example was slightly misleading and a
larger change than necessary. I made a new diff to show smaller example
rule change:
https://www.diffchecker.com/Ba5a0agP




On Tue, Sep 21, 2021 at 5:47 PM Jacques Nadeau  wrote:

> In the original creation of ImmutableBeans [1], there was a discussion of
> using Immutables instead of something custom to do the configuration pojos.
> Because no one did the work of integrating one of those off the shelf
> packages at the time, we just went forward with the custom ImmutableBeans.
> When I started working on supporting AOT compilation (specifically GraalVM,
> CALCITE-4786) it became clear that the ways that the proxies are used in
> Immutable beans are fairly difficult to make work correctly. (Especially
> the evil ImmutableBeans.copy operation.)
>
> Because of this, I opened up CALCITE-4787 [2] to replace ImmutableBeans
> with the great Immutables package [6]. It should ultimately reduce the code
> we have to maintain while providing an extended set of well maintained
> capabilities. It uses an AnnotationProcessor to generate real code so you
> can actually look at any immutable/builder classes and is all done at
> compile time so performance is exceptional. The change feels like a win/win
> (better maturity, less api surface area, ahead of time compilation) but I
> wanted to make sure people were onboard with it. I've opened an initial PR
> which shows the conversion for one class [3]. As you should be able to see,
> the changes are fairly minimal to the code and configuration APIs don't
> change.
>
> There are ~40 classes that directly reference ImmutableBeans so this
> change will do small amounts of changes to each but should not impact
> functionality/behavior. However, the use of RelRule.Config.EMPTY actually
> means that all rule files need a small change in them (even if they don't
> directly reference ImmutableBeans). More details below on why this change
> is necessary. Needless to say I believe it is minor and given the benefits
> it is worth it. That being said, it will be a breaking change. As such, I
> wanted to hear people's feedback before doing all the boring minor code
> changes.
>
> Are people okay with the breaking change? To give people an example of how
> a rule would change, here is an example in a diff [4].
>
> Thanks,
> Jacques
>
> ---
> More details on the differences between the two systems and why small rule
> changes are necessary:
>
>- Item 1: ImmutableBeans is actually quite lenient on the construction
>of a proxy object (due to no separation of build vs copy). If a user
>doesn't populate a non-nullable field, ImmutableBeans doesn't actually
>declare any problem until the specific property is retrieved. On the other
>hand, Immutables is much stricter and has a clear distinction between build
>and copy (e.g. withFooProperty() type methods). If a property is abstract
>(no default method) and is not marked nullable, it is impossible in
>Immutables to construct a concrete POJO of that class. An example of this
>weird "broken class availability" is here [5] in the invocation chain.
>- Item 2: ImmutableBeans.copy (and the RelRule.Config.as() operation
>is best described as an unsafe cast. It works when used in the constrained
>example of interface subclassing by copying the invocation handlers to a
>new dynamic proxy. This is somewhat anti-pattern since most problems
>wouldn't occur until runtime. This pattern seems to exist in ImmutableBeans
>because it doesn't seem to distinguish between construction and copying. In
>Immutables you have the option to handle this two different ways: default
>values are always declared on the properties, possibly overridden OR
>through the use of a partial builder.
>
>
> [1] https://issues.apache.org/jira/browse/CALCITE-3328
> [2] https://issues.apache.org/jira/browse/CALCITE-4787
> [3] https://github.com/apache/calcite/pull/2531
> [4] https://www.diffchecker.com/zqTmEKdz
> [5]
> https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/ProjectFilterTransposeRule.java#L234
> [6] https://immutables.github.io/
>
>

[jira] [Created] (CALCITE-4790) Make Gradle pass the 'user.timezone' property to the test JVM

[Alessandro Solimando (Jira)]

Alessandro Solimando created CALCITE-4790:
-

 Summary: Make Gradle pass the 'user.timezone' property to the test 
JVM
 Key: CALCITE-4790
 URL: https://issues.apache.org/jira/browse/CALCITE-4790
 Project: Calcite
  Issue Type: Improvement
Affects Versions: 1.27.0
Reporter: Alessandro Solimando
Assignee: Alessandro Solimando


Tests run in the forked JVM, only the explicit set of properties is passed 
there, which are listed 
[here|https://github.com/apache/calcite/blob/4b1e9ed1a513eae0c873a660b755bb4f27b39da9/build.gradle.kts#L711-L726]

Add -Duser.timezone to the list of properties to be passed along.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Re: [CALCITE-4232]-Elasticsearch IN Query is not supported

[Andrei Sereda]

Hi Arpit,

I'm also happy to take a look at your PR since I'm more or less familiar
with that part of the codebase.

Thanks,
Andrei



On Wed, Sep 22, 2021, 06:17 Stamatis Zampetakis  wrote:

> Hi Arpit,
>
> Given that it has been a long time since the last review there are quite a
> few things that changed in Calcite.
> I would suggest rebasing the PR based on the current master and I will try
> to have another look.
>
> Best,
> Stamatis
>
>
> On Mon, Sep 20, 2021 at 3:30 PM Arpit Motwani
>  wrote:
>
> > Hi Community,
> >
> > Please find below details of issue which we are facing in a pull request
> > raised by us for changes in ElasticsearchAdapters
> >
> > Issue Id- CALCITE-4232<
> https://issues.apache.org/jira/browse/CALCITE-4232>
> >
> > Above change has been accepted but is not merged upstream, is something
> > pending from our side ?
> >
> > Also, we see below error in PR of this change-
> >
> > This branch cannot be rebased due to conflicts
> >
> > Do we need to do something for the rebasing conflict? If yes kindly tell
> > us the approach for resolving this issue.
> >
> > Thanks & Regards
> >  Arpit Motwani
> >
> >
> > 
> >
> >
> >
> >
> >
> >
> > NOTE: This message may contain information that is confidential,
> > proprietary, privileged or otherwise protected by law. The message is
> > intended solely for the named addressee. If received in error, please
> > destroy and notify the sender. Any use of this email is prohibited when
> > received in error. Impetus does not represent, warrant and/or guarantee,
> > that the integrity of this communication has been maintained nor that the
> > communication is free of errors, virus, interception or interference.
> >
>

Re: [CALCITE-4232]-Elasticsearch IN Query is not supported

[Stamatis Zampetakis]

Hi Arpit,

Given that it has been a long time since the last review there are quite a
few things that changed in Calcite.
I would suggest rebasing the PR based on the current master and I will try
to have another look.

Best,
Stamatis


On Mon, Sep 20, 2021 at 3:30 PM Arpit Motwani
 wrote:

> Hi Community,
>
> Please find below details of issue which we are facing in a pull request
> raised by us for changes in ElasticsearchAdapters
>
> Issue Id- CALCITE-4232
>
> Above change has been accepted but is not merged upstream, is something
> pending from our side ?
>
> Also, we see below error in PR of this change-
>
> This branch cannot be rebased due to conflicts
>
> Do we need to do something for the rebasing conflict? If yes kindly tell
> us the approach for resolving this issue.
>
> Thanks & Regards
>  Arpit Motwani
>
>
> 
>
>
>
>
>
>
> NOTE: This message may contain information that is confidential,
> proprietary, privileged or otherwise protected by law. The message is
> intended solely for the named addressee. If received in error, please
> destroy and notify the sender. Any use of this email is prohibited when
> received in error. Impetus does not represent, warrant and/or guarantee,
> that the integrity of this communication has been maintained nor that the
> communication is free of errors, virus, interception or interference.
>

Re: COALESCE returns maximum of available CHAR capacity.

[stanilovsky evgeny]

Julian, thanks !

mysql differs here:
mysql> SELECT LENGTH(a) from (SELECT COALESCE('a', '') as a) as t;
+---+
| LENGTH(a) |
+---+
| 1 |
+---+

mysql> SELECT LENGTH(a) from (SELECT COALESCE('aa', '') as a) as t;
+---+
| LENGTH(a) |
+---+
| 2 |
+---+

I don`t claim that calcite need to be the same, i just want to understand  
what is correct.
My point of view - if COALESCE transforms into CASE-WHEN statement and  
returns first not null it strange instead of expected 'a' gets 'a '.

I`m wrong ?

The current behavior looks ok to me. The expression has type CHAR(2)  
because the arguments have types CHAR(1) and CHAR(2). So ‘a’ is widened  
to ‘a ‘.


Julian

On Sep 22, 2021, at 12:00 AM, stanilovsky evgeny  
 wrote:


sorry for typo, 'fill the ticket' of course ))


hi community !
I found that COALESCE('a', 'bb') will return 'a ' <-- whitespace is  
present, i found that now it expands for maximum presented CHAR  
(RelDataType#inferReturnType logic)

sql 92 standard tolds:

'''
COALESCE (V1, V2) is equivalent to the following :
CASE WHEN V1 IS NOT NULL THEN V1 ELSE V2 END
'''

So i suppose that existing behavior is erroneous, if it`s ok i fell  
the ticket.


wdyt ?

Re: COALESCE returns maximum of available CHAR capacity.

[Julian Hyde]

The current behavior looks ok to me. The expression has type CHAR(2) because 
the arguments have types CHAR(1) and CHAR(2). So ‘a’ is widened to ‘a ‘.

Julian

> On Sep 22, 2021, at 12:00 AM, stanilovsky evgeny  
> wrote:
> 
> sorry for typo, 'fill the ticket' of course ))
> 
>> hi community !
>> I found that COALESCE('a', 'bb') will return 'a ' <-- whitespace is present, 
>> i found that now it expands for maximum presented CHAR 
>> (RelDataType#inferReturnType logic)
>> sql 92 standard tolds:
>> 
>> '''
>> COALESCE (V1, V2) is equivalent to the following :
>> CASE WHEN V1 IS NOT NULL THEN V1 ELSE V2 END
>> '''
>> 
>> So i suppose that existing behavior is erroneous, if it`s ok i fell the 
>> ticket.
>> 
>> wdyt ?

Re: COALESCE returns maximum of available CHAR capacity.

[stanilovsky evgeny]

sorry for typo, 'fill the ticket' of course ))


hi community !
I found that COALESCE('a', 'bb') will return 'a ' <-- whitespace is  
present, i found that now it expands for maximum presented CHAR  
(RelDataType#inferReturnType logic)

sql 92 standard tolds:

'''
COALESCE (V1, V2) is equivalent to the following :
CASE WHEN V1 IS NOT NULL THEN V1 ELSE V2 END
'''

So i suppose that existing behavior is erroneous, if it`s ok i fell the  
ticket.


wdyt ?