Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes several path tests according to the PartiQL Specification #116

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Jul 12, 2024

Description

  • Fixes several path tests according to the PartiQL Specification

Note: Please click "Hide Whitespace" on the Files Changed tab. The file was formatted oddly, so I allowed my IDE to auto-format.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@yliuuuu yliuuuu self-requested a review July 16, 2024 18:21
Copy link
Contributor

@yliuuuu yliuuuu left a comment

Choose a reason for hiding this comment

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

I tagged duplicated comments "Same as above" to prevent losing track of the test case.

Comment on lines +191 to +194
evalMode: [
EvalModeCoerce,
EvalModeError
],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really weird...

Build up on the previous test cases:

If MISSING.a is MISSING failed evalMode: EvalModeError, why should NULL.a succeed?

I understand you can coerce NULL to any type, but it does seem weird to me ....

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR doesn't change anything here -- it's a formatting diff.

Though, to answer with my own opinion, all SQL binary operations return null when one of the operands is the null value. AKA NULL + 1 = NULL, 1 - NULL = NULL... and, IMO, NULL.a = NULL. MISSING is a type error, and upon passing it to (almost) all functions, it results in a type error.

Though, this PR isn't adjusting these tests. It's formatting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nonblocking: but we can think of path navigation as a function that is not nullCall?

Comment on lines +446 to +448
{
evalMode: EvalModeError,
result: EvaluationFail
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate on why this failed?

SELECT VALUE v1
FROM s as v1

Copy link
Member Author

Choose a reason for hiding this comment

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

From PartiQL Specification:

The expression e[∗] reduces to (i.e., is equivalent to):
SELECT VALUE v FROM e AS v

And,

In the following cases the expression in the FROM clause item has the wrong type. Under the type checking option, all of these cases raise an error and the query fails. Under the permissive option, the cases proceed as follows
-- Section 5.1.1: Mistyping Cases

Therefore,

<scalar>[*] = SELECT VALUE v FROM <scalar> AS v --> FROM clause item has the wrong type.

Comment on lines +463 to +465
{
evalMode: EvalModeError,
result: EvaluationFail
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate on why this failed?

SELECT VALUE v1 
FROM UNPIVOT s as v1

Copy link
Contributor

Choose a reason for hiding this comment

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

Spec section 5.1.1 - Iteration over a scalar value

FROM s AS V
where s is a scalar value. Then s coerces into the bag << s >>.

This section seems to be mode independent, i.e., coercion always happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the top of Section 5.1.1:

In the following cases the expression in the FROM clause item has the wrong type. Under the type checking option, all of these cases raise an error and the query fails. Under the permissive option, the cases proceed as follows...

Comment on lines +480 to +482
{
evalMode: EvalModeError,
result: EvaluationFail
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment.

Comment on lines +497 to +499
{
evalMode: EvalModeError,
result: EvaluationFail
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment.

Comment on lines +523 to +525
{
evalMode: EvalModeError,
result: EvaluationFail
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment.

Comment on lines +541 to +543
{
evalMode: EvalModeError,
result: EvaluationFail
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment.

Comment on lines +755 to +756
evalMode: EvalModeCoerce,
output: $missing::null
Copy link
Contributor

Choose a reason for hiding this comment

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

Tuple navigation with array notation The expression a[s] is a shorthand for the tuple path navigation a.s when
the expression s is either (a) a string literal or (b) an expression that is explicitly CAST into a string. For example:
{'a': 1, 'b': 2}['a'] ⇔ {'a': 1, 'b':2}.'a' → 1

Not sure what the single quoted 'a' means in our spec. but curious why you think this should return a MISSING...

Copy link
Member Author

Choose a reason for hiding this comment

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

when the expression s is either (a) a string literal or (b) an expression that is explicitly CAST into a string... If s is not a string literal or an expression that is cast into a string, then a[s] is evaluated as an array path navigation.

So, some_struct['a'] (literal string 'a') results in a path expression. Similarly, some_struct[CAST('a' || 'b' AS STRING)] (explicit cast) results in a path expression.

"If s is not a string literal or an expression that is cast into a string" (like variable references, plus operations, etc), "then a[s] is evaluated as an array path navigation." All of the below evaluate as an array path navigation:

  1. some_struct[some_variable]
  2. some_struct[1 + 1]
  3. some_struct['hello' || ', world']

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotya. So test_struct['unambiguous_variable'] => 'this is unambiguous_lookup_field'`. And the test case is missing single quote here. Is this understanding correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I think the intention of the test is a bit different. unambiguous_variable is a global variable pointing to some string, and I believe the original intention was to test that the underlying value that the variable points to can be used to key into structs -- though that assumption is wrong according to the specification.

Comment on lines +772 to +773
evalMode: EvalModeCoerce,
output: $missing::null
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

See comment.

Comment on lines +787 to +789
evalMode: EvalModeCoerce,
output: $missing::null
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

See comment.

Copy link
Contributor

@yliuuuu yliuuuu left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying.

Comment on lines +191 to +194
evalMode: [
EvalModeCoerce,
EvalModeError
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nonblocking: but we can think of path navigation as a function that is not nullCall?

@johnedquinn johnedquinn merged commit 52542f9 into main Jul 16, 2024
4 checks passed
@johnedquinn johnedquinn deleted the main-fix-missing-attr branch July 16, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants