date:20181218

Change in asterixdb[master]: [NO ISSUE][API] added parse-only request parameter

2018-12-18 Thread Till Westmann (Code Review)

Till Westmann has posted comments on this change.

Change subject: [NO ISSUE][API] added parse-only request parameter
..

Patch Set 1:

(13 comments)

Looks good.
I added a few suggestions to simply this a bit more - mainly by returning the
result object in the "result" field instead of adding a new top-level field to
the result envelope. I might be contradicting myself here, but looking at this
change it seems that using the result field make more sense.

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java
File
asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java:

Line 54: PARSEVALS("parsevals");
> We need to come up with a better name than "parsevals". Let's discuss with
"parsevals" seems a bit generic,
But thinking about this a little more, I think that we don't need a new field.
I would claim that the statement parameters are a valid result of a parse-only
request and that we should return the parameters in the result field. The is
very similar to the logical plan that one will get when running an "EXPLAIN .."
query.

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java
File
asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java:

Line 534: // CORS
> let's extract CORS header handling into a separate method and call it from
Yes, that makes sense. I thought that we already had that factored out at some
point (but maybe that's just wishful thinking ...).

Line 584: throw new CompilationException("MultiStatement not
support"); //TODO: proper exception message
> We need to fix this TODO item.
We should probably use QueryTranslator.validateStatements for this.
Interestingly that throws a RuntimeDataException while it looks more like a
CompilationException to me. We should fix that (either in this change or in a
separate change).

PS1, Line 586: 0
And we'd need to get the last statement here.

PS1, Line 691: printParseValues
Rename to "printParseOnlyResult"?

PS1, Line 694: PARSEVALS
Another benefit of using the results filed would be that we probably don't need
most of the additions in the test framework.

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ResultUtil.java
File
asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ResultUtil.java:

PS1, Line 384: ParseValuesJsonPrintUtils
I think that we don't need this nested class.
Wondering if it'd work to move this into ExecutionPlansJsonPrintUtil. The name
doesn't seem to be a perfect match, but I think that printing information from
the parse tree is pretty close to writing plans.
Otherwise we could also create a new standalone class next to
ExecutionPlansJsonPrintUtil.

PS1, Line 399: quoteAndEscape
I think that we should not quote and escape here. If we have an array inside a
string field, the consumer of the result will have to first parse the JSON to
get the string and the to parse the sting again. Instead, we should have an
array of strings.

PS1, Line 404: TODO
Indeed

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java
File
asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java:

PS1, Line 157: PARSE_QUERY_TYPE
E.g. we wouldn't need this, if we return the information in the result field
(but we'd need to rename the test files).

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ParseOnlyTest.java
File
asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ParseOnlyTest.java:

PS1, Line 34: TODO
Do we still need this?

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseOnlyResult.java
File
asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseOnlyResult.java:

PS1, Line 39: toStringDescription
Rename this as toJson()? We're using this in other places for methods that
return a JSON object as a string.

https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java
File
asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java:

PS1, Line 261: TODO
I think that it's fine in the rewriter.

--
To view, visit https://asterix-gerrit.ics.uci.edu/3085
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd2f461c22b05a5fcaa50a6e4f9b7dcd91acc184
Gerrit-PatchSet: 1
Gerrit-P

Change in asterixdb[master]: [NO ISSUE][EXT] Fix JRecord setField for open fields

2018-12-18 Thread Xikui Wang (Code Review)

Xikui Wang has submitted this change and it was merged.

Change subject: [NO ISSUE][EXT] Fix JRecord setField for open fields
..


[NO ISSUE][EXT] Fix JRecord setField for open fields

- user model changes: no
- storage format changes: no
- interface changes: no

Details:
setField method should be able to handle open fields.

Change-Id: I4ec185d62e78e8100215334bc656586268314797
Reviewed-on: https://asterix-gerrit.ics.uci.edu/3068
Sonar-Qube: Jenkins 
Tested-by: Jenkins 
Integration-Tests: Jenkins 
Contrib: Jenkins 
Reviewed-by: Ali Alsuliman 
---
M 
asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.1.ddl.sqlpp
M 
asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JRecord.java
2 files changed, 2 insertions(+), 7 deletions(-)

Approvals:
  Anon. E. Moose #1000171: 
  Ali Alsuliman: Looks good to me, approved
  Jenkins: Verified; No violations found; ; Verified



diff --git 
a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.1.ddl.sqlpp
 
b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.1.ddl.sqlpp
index 4402130..464ecb2 100644
--- 
a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.1.ddl.sqlpp
+++ 
b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.1.ddl.sqlpp
@@ -35,8 +35,7 @@
 };
 
 create type DetectResultType as open{
-id:int64,
-sensitive: boolean
+id:int64
 };
 
 create dataset EmpDataset(InputRecordType) primary key id;
diff --git 
a/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JRecord.java
 
b/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JRecord.java
index 7fa9d0b..10c228d 100644
--- 
a/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JRecord.java
+++ 
b/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JRecord.java
@@ -99,11 +99,7 @@
 if (pos >= 0) {
 fields[pos] = fieldValue;
 } else {
-if (openFields.get(fieldName) != null) {
-openFields.put(fieldName, fieldValue);
-} else {
-throw new 
RuntimeDataException(ErrorCode.LIBRARY_JAVA_JOBJECTS_UNKNOWN_FIELD, fieldName);
-}
+openFields.put(fieldName, fieldValue);
 }
 }
 

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/3068
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4ec185d62e78e8100215334bc656586268314797
Gerrit-PatchSet: 5
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Xikui Wang 
Gerrit-Reviewer: Ali Alsuliman 
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Jenkins 
Gerrit-Reviewer: Xikui Wang

Change in asterixdb[master]: [NO ISSUE][EXT] Fix JRecord setField for open fields

2018-12-18 Thread Ali Alsuliman (Code Review)

Ali Alsuliman has posted comments on this change.

Change subject: [NO ISSUE][EXT] Fix JRecord setField for open fields
..


Patch Set 4: Code-Review+2

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/3068
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ec185d62e78e8100215334bc656586268314797
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Xikui Wang 
Gerrit-Reviewer: Ali Alsuliman 
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Jenkins 
Gerrit-HasComments: No

Change in asterixdb[master]: [NO ISSUE][EXT] Fix JRecord setField for open fields

2018-12-18 Thread Anon. E. Moose (Code Review)

Anon. E. Moose #1000171 has posted comments on this change.

Change subject: [NO ISSUE][EXT] Fix JRecord setField for open fields
..


Patch Set 4: Contrib+1

Analytics Compatibility Tests Successful
https://goo.gl/CySqUX : SUCCESS

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/3068
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ec185d62e78e8100215334bc656586268314797
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Xikui Wang 
Gerrit-Reviewer: Ali Alsuliman 
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Jenkins 
Gerrit-HasComments: No

27 matches

Mail list logo