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

PHOENIX-7155 Validate Partial Index support with JSON #1767

Merged
merged 5 commits into from Dec 21, 2023

Conversation

ranganathg
Copy link
Contributor

@ranganathg ranganathg marked this pull request as ready for review December 19, 2023 12:14
Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

Haven't had a chance to look at the patch, but wanted to point out that we have Apache License failures on this PR coming from the new JSON files.

@ranganathg
Copy link
Contributor Author

Haven't had a chance to look at the patch, but wanted to point out that we have Apache License failures on this PR coming from the new JSON files.

Thanks @gjacoby126 . As per my understanding - The JSON specification does not include comments, and they cannot be used in JSON files.

Copy link
Contributor

@kadirozde kadirozde left a comment

Choose a reason for hiding this comment

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

We should also add JSON specific tests to WhereCompilerTest#testWhereInclusion().

@kadirozde
Copy link
Contributor

Haven't had a chance to look at the patch, but wanted to point out that we have Apache License failures on this PR coming from the new JSON files.

Thanks @gjacoby126 . As per my understanding - The JSON specification does not include comments, and they cannot be used in JSON files.

We can eliminate the json file and use a static map (see BaseTest) or an array instead.

@ranganathg
Copy link
Contributor Author

ranganathg commented Dec 20, 2023

We can eliminate the json file and use a static map (see BaseTest) or an array instead.

I would prefer to have json data in separate json files itself. If it's within a Java file, its not readable and IDE support to modify JSON with java static map wouldn't be clean.
Does that make sense?

@ranganathg
Copy link
Contributor Author

We should also add JSON specific tests to WhereCompilerTest#testWhereInclusion().

Pushed new changes for this.

// Add rows to the data table before creating a partial index to test that the index
// will be built correctly by IndexTool
conn.createStatement().execute(
"upsert into " + dataTableName + " values ('id1', 70, 2, 3.14, 'a','" + getJsonString(
Copy link
Contributor

Choose a reason for hiding this comment

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

To improve the readability of the tests, we should include the json doc payload directly in the string for the upsert statement instead of retrieving from a file. The json doc should include only the content necessary for this test. For example, for this test the string for the doc could be ‘{"info": {"address ": {"exits" : true}}}’. If the same json string is used multiple times then we can define a string constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure makes sense. Will make these changes

@kadirozde
Copy link
Contributor

I would prefer to have json data in separate json files itself. If it's within a Java file, its not readable and IDE support to modify JSON with java static map wouldn't be clean. Does that make sense?

It is not easy to read the test when the input is in a non-java file that cannot be accessed with a shortcut or link. If the input is large then this is acceptable but for a tiny input, it is inconvenient.

@gjacoby126
Copy link
Contributor

@ranganathg - If you keep the JSON, exceptions to the license check can be added here: https://github.com/apache/phoenix/blob/master/pom.xml#L636

@ranganathg
Copy link
Contributor Author

@ranganathg - If you keep the JSON, exceptions to the license check can be added here: https://github.com/apache/phoenix/blob/master/pom.xml#L636

This is nice. Thank you @gjacoby126

@ranganathg
Copy link
Contributor Author

It is not easy to read the test when the input is in a non-java file that cannot be accessed with a shortcut or link. If the input is large then this is acceptable but for a tiny input, it is inconvenient.

Took your comment and this made the code much simpler and easy to read. Thank you :)

Copy link
Contributor

@kadirozde kadirozde left a comment

Choose a reason for hiding this comment

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

+1, Thanks!

@kadirozde kadirozde merged commit ead8f05 into apache:PHOENIX-628-feature Dec 21, 2023
1 check passed
ranganathg added a commit to ranganathg/phoenix that referenced this pull request Jan 3, 2024
ranganathg added a commit to ranganathg/phoenix that referenced this pull request Jan 3, 2024
* PHOENIX-628 - Support native JSON data type (apache#1667)

1. Update the SQL Exception code to reflect ComparisonNotSupportedException
2. Changed function name from Equality to Comparison.

* PHOENIX-7058 : Implement json_query function on the json object (apache#1696)

* PHOENIX-7044 : Support Index on a generated column that extracts a scalar value from JSON column (apache#1708)

* PHOENIX-7099 : Implement JSON_EXISTS function on json object (apache#1732)

* PHOENIX-7099 : Implement JSON_EXISTS function on json object

* PHOENIX-7073 : Parse JSON columns on the server side (apache#1712)

* PHOENIX-7073 : Parse JSON columns on the server side

* PHOENIX-7155 Validate Partial Index support with JSON (apache#1767)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants