bitflicker64 commented on code in PR #833:
URL:
https://github.com/apache/hugegraph-toolchain/pull/833#discussion_r4353371012
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/util/ReadOnlyGremlin.java:
##########
@@ -72,6 +82,13 @@ public static void validate(String query) {
if (parser.position != query.length()) {
throw invalid();
}
+ int insertion = parser.consumerFollowed ? -1 :
+ parser.consumerPosition >= 0 ? parser.consumerPosition
: query.length();
Review Comment:
Minor: Scalar reads now get the default limit and the "no result limit"
notice. At b44b31bb, `g.V().count()` and `g.V().values('age').sum()` were left
unchanged. Now the insertion point is the end of the statement, so they become
`...count().limit(300)` and `...sum().limit(300)`. `QueryService` then sets
`default_limit_applied`, and the page shows "No result limit was set; This
query defaults to at most 300 results" above a single count. The count is
correct, but the notice suggests it may be capped at 300.
`QueryServiceTest.testScalarOutputLimitsFollowTheReductionBeforeDispatch`
asserts the new dispatch, so the limit is intended. The notice is the problem.
A terminal `count`, `sum`, `min`, `max` or `mean` at depth 0 could skip the
insertion, as at b44b31bb. Cypher `RETURN count(n)` has the same notice, which
predates this delta. Or show the notice only when the row count reaches
`default_limit`, which covers Cypher too.
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/util/GremlinUtil.java:
##########
@@ -464,56 +460,31 @@ private static boolean chainContinues(String gremlin, int
newline) {
}
private static String optimizeStatement(String statement,
- String matchable, int limit) {
+ String matchable, int limit,
+ boolean preview) {
int codeEnd = matchable.length();
while (codeEnd > 0 && Character.isWhitespace(
matchable.charAt(codeEnd - 1))) {
codeEnd--;
}
String code = matchable.substring(0, codeEnd);
- boolean ignored = IGNORED_PATTERNS.stream().anyMatch(pattern -> {
- return pattern.matcher(code).find();
- });
- if (ignored) {
+ ReadOnlyGremlin.Syntax syntax;
+ try {
+ // Reuse read-only validation and shallow call positions. Unknown
+ // manual syntax and mutations retain their native execution path.
+ syntax = ReadOnlyGremlin.parse(
+ code.replace('\n', ' ').replace('\r', ' '));
+ } catch (RuntimeException ignored) {
return statement;
}
- for (Pattern pattern : LIMIT_PATTERNS) {
- Matcher matcher = pattern.matcher(code);
- if (matcher.find()) {
- return statement.substring(0, codeEnd) + ".limit(" + limit +
- ")" + statement.substring(codeEnd);
- }
+ if (preview && syntax.collection()) {
+ throw new
ExternalException("gremlin.preview.collection_requires_async");
}
- return statement;
- }
-
- private static Set<Pattern> compile(Set<String> texts) {
- Set<Pattern> patterns = new LinkedHashSet<>();
- for (String text : texts) {
- String regex = StringUtils.replaceEach(text, COMPILE_SEARCH_LIST,
- COMPILE_TARGET_LIST);
- Pattern pattern;
- // Assume that (STR), (NUM) and () not exist at the same time
- if (text.contains("(STR)")) {
- // single quote
- pattern = compile(regex.replaceAll("STR", "'[\\\\s\\\\S]+'"));
- patterns.add(pattern);
- // double quotes
- pattern = compile(regex.replaceAll("STR",
"\"[\\\\s\\\\S]+\""));
- patterns.add(pattern);
- } else if (text.contains("(NUM)")) {
- pattern = compile(regex.replaceAll("NUM", "[\\\\d]+"));
- patterns.add(pattern);
- } else if (text.contains("()")) {
- pattern = compile(regex);
- patterns.add(pattern);
- }
+ int position = syntax.limitPosition();
+ if (position < 0 || !preview && (syntax.explicitLimit() ||
syntax.collection())) {
Review Comment:
Minor: Outside preview, any depth-0 `limit` now turns off the default limit,
even when it bounds only the input. `g.V().limit(10).out()` and
`g.V().hasLabel('person').limit(100).both().both()` come back unchanged. On
master the `.out()`/`.both()` suffix rule appended `.limit(300)`, and b44b31bb
did too.
Preview requests still get `.limit(1001)`, so the analysis page stays
bounded, at 1,000 rows instead of 300. The non-preview callers are not:
`LangChainController` `/gremlin` uses `executeSingleGremlinQuery`, and
`SaasGraphViewController` uses `executeGremlinQuery`. Neither caps rows, so a
multi-hop expansion after a small limit now returns its full result. Consider
clearing `explicitLimit` in `Parser.chain` when a later depth-0 step expands
the stream (`out`, `in`, `both` and their `E`/`V` forms), as the master rule
did. A last-step-only rule would instead cap `g.V().limit(500).valueMap()` at
300.
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/util/ReadOnlyGremlin.java:
##########
@@ -82,16 +99,32 @@ private static final class Parser {
private final String text;
private int position;
+ private boolean explicitLimit;
+ private boolean collection;
+ private int consumerPosition = -1;
+ private boolean consumerFollowed;
private Parser(String text) {
this.text = text;
}
private void chain(Set<String> methods, int depth) {
while (this.consume('.')) {
- if (!methods.contains(this.identifier())) {
+ int dotPosition = this.position - 1;
+ String name = this.identifier();
+ if (!methods.contains(name)) {
throw invalid();
}
+ if (methods == STEPS) {
+ this.collection |= COLLECTIONS.contains(name);
+ if (depth == 0) {
+ this.explicitLimit |= "limit".equals(name);
Review Comment:
Minor: Only `limit` counts as an explicit bound here, so `range` and `tail`
get the default too. `g.V().range(0,5000)` becomes
`g.V().range(0,5000).limit(300)`, and `g.V().tail(500)` returns 300 rows with
the "No result limit was set" notice. master and b44b31bb left both unchanged,
and README line 92 says the default applies only without an explicit bound.
Matching `limit`, `range` and `tail` here would fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
