Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Master apache #719

Open
wants to merge 4 commits into
base: master-Apache
Choose a base branch
from

Conversation

ramkrish86
Copy link
Contributor

Changes for throwing exception for queries with upsert select with specific index.
Tries not to set the maxlength in column def for array type as we don't really know the actual maxlength for arrays during definition.

{
if(lsq != null)
{
throwRecognitionException(lsq);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is always an error, then we wouldn't want it in the syntax, but I'm actually capturing this information now and storing it in an ARRAY_SIZE key value column on the column metadata row. We're not using it, but we should use it down the road.

With our dynamic column feature, you can define a column at upsert time and populate it. This would include arrays. I think for arrays, you should never have a null dt. I think it's just a matter of not having both the dt part and the array brackets be optional - instead the whole thing should be optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for now I will revert this change? I was just wondering how that
bq.(lsq=LSQUARE (a=NUMBER)? RSQUARE)?
was added. I was thinking I added that in my previous commit. So you don't want that to be solved in the syntax level? Ok will see then what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you switch to the Apache git repo as the github repo is no longer
being used - your on the phoenix dev list, right and have been getting my
mail? If not please join here:
http://phoenix.incubator.apache.org/mailing_list.html

Get the latest from the Apache git repo and you'll see my fix - it was just
modifying the syntax slightly.

For PDataType.getByteSize() should return null if the type doesn't know how
big it is, which is fine for arrays. For the Expression (i.e. this would be
a KeyValueColumnExpression specifically), the getByteSize() has all the
information it needs to calculate the size correctly.

@jtaylor-sfdc
Copy link
Contributor

@ramkrish86 - I fixed this in the master branch in our Apache git (I'd point you to the commit, but our github mirrors aren't up yet). If you could take a look at the following, that would be good:

  • More testing around arrays:
    • usage of ArrayConstructorExpression in a WHERE clause that doesn't evaluate to a constant. Include a test that uses multiple column references like WHERE a_array = ARRAY[1, col1, 2, col2], as this will execute "partial" evaluation during filter evaluation time (see slightly tweaked logicin ArrayConstructorExpression.evaluate).
    • usage of ArrayConstructorExpression that requires the values to be coerced to a different type, such as WHERE a_array = ARRAY[1.2, col1 CAST AS DOUBLE, 2, col2 CAST AS LONG] to confirm that the types are coerced to DECIMAL. Note that we should coerce any constants in advance, in ExpressionCompiler, so we don't end up coercing them at runtime.
    • usage of ArrayConstructorExpression with null values. I made is more forgiving for now - nulls correspond to zero for numeric types. Need a few tests around this, though.
    • should we allow ARRAY[1.2, 'foo', 3], as a common base type could always be VARBINARY? Might be a bit odd, as you'd potentially be comparing the bytes from different types, but we should see what Postgres allows here as a model to follow. IMHO, this work is lower priority than the next one, though.
  • Implement Push projection of a single ARRAY element to the server #669.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants