[Impala-ASF-CR] IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py"
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12096 ) Change subject: IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 Gerrit-Change-Number: 12096 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 07:16:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py"
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12096 ) Change subject: IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 Gerrit-Change-Number: 12096 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 05:02:55 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 04:46:38 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3573/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 04:46:39 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 2: Failure seemed to look flaky to me; retrying before digging in deeper. https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/3887/testReport/junit/failure.test_failpoints/TestFailpoints/test_lifecycle_failures/ -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 04:46:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: > > To what extent is this the Jackson streaming API? > > The Jackson Streaming API is low level, while the serialization API > provided here operates at a higher level, making assumptions about > the kinds of data we want to serialize. > > Jackson is very good for producing standard JSON. > > To show the relationship, went ahead and implemented a serializer > based on Jackson. The code is actually longer than the bespoke > version, but the Jackson version produces standard JSON we can > export to other tools. Tell me more about what you're trying to do? If we're going to call it JSON, it should probably be parseable with most JSON parsers. Various Hadoop ecosystem things use plenty of Jackson and class annotations and what-not. It's not perfect, but it seems pretty standard. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 04:16:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@83
PS3, Line 83: objectList
shouldn't this be stringList?
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@88
PS3, Line 88: objectList
should this be scalarList?
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@34
PS3, Line 34: public ToJsonOptions showSource(boolean flag)
: { showSource_ = flag; return this; }
Sorry if I wasn't clear. I meant to use the standard style.
public ToJsonOptions showSource(boolean flag) {
showSource_ = flag;
return this;
}
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java
File fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@39
PS2, Line 39:
: protected abstract void build();
:
: protected void verify(String expected) {
: builder_.close();
: assertEquals(expected, strWriter_.toString());
: }
> The style is common in tests to avoid accidental reuse of common variables
What do you think about this?
@FunctionalInterface
private interface Verifier {
void verify(String expected);
}
private static Verifier testBuilder(Consumer consumer) {
StringWriter strWriter = new StringWriter();
TreeBuilder builder = new TreeBuilder(new PrintWriter(strWriter));
consumer.accept(builder);
return (expected) -> {
builder.close();
assertEquals(expected, strWriter.toString());
};
}
public static Verifier testFormatter(ToJsonOptions options,
Consumer consumer) {
JsonTreeFormatter fmt = new JsonTreeFormatter(options);
fmt.build();
consumer.accept(fmt);
return (expected) -> {
assertEquals(expected, fmt.toString());
};
}
public static Verifier testFormatter(Consumer consumer) {
return testFormatter(ToJsonOptions.full(), consumer);
}
And we can use it like this:
testBuilder(builder -> {
assertEquals(1, builder.root());
}).verify("{\n}\n");
testFormatter(fmt -> {
ObjectSerializer os = fmt.root();
os.field("bool", true);
}).verify("{\n bool: true\n}\n");
IMO lambda is generally more readable for creating a DSL (and generates better
code because of invokedynamic, although I don't think it matters for this unit
test) than using anonymous inner class.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@65
PS2, Line 65:
> Would love to do it that way if Java had something like a "HERE" doc. But,
I'm too pampered with IntelliJ that can nicely preserve the formatting for
String multi-line :) Anyway, I'm okay with this.
--
To view, visit http://gerrit.cloudera.org:8080/12079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c
Gerrit-Change-Number: 12079
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Sat, 15 Dec 2018 04:15:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1623/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 04:15:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: > To what extent is this the Jackson streaming API? The Jackson Streaming API is low level, while the serialization API provided here operates at a higher level, making assumptions about the kinds of data we want to serialize. Jackson is very good for producing standard JSON. To show the relationship, went ahead and implemented a serializer based on Jackson. The code is actually longer than the bespoke version, but the Jackson version produces standard JSON we can export to other tools. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 03:36:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java File fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java: http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java@204 PS4, Line 204: _objectFieldValueSeparatorWithSpaces = DEFAULT_SEPARATORS.getObjectFieldValueSeparator() + " "; line too long (101 > 90) http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java File fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java: http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java@22 PS4, Line 22: import org.apache.impala.common.serialize.JacksonTreeSerializer.JacksonTreeStringSerializer; line too long (92 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 03:29:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 3:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java
File
fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@26
PS2, Line 26: /**
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@28
PS2, Line 28:*/
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@51
PS2, Line 51: public void object(String name, JsonSerializable obj) {
: if (obj != null) obj.serialize(this);
: else if (!options().elide()) scalar(name, null);
: }
> nit:
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@35
PS2, Line 35: /**
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@45
PS2, Line 45: }
> I may miss something obvious, but I don't see anything that requires suppre
JSON-Simple is old school: it's objects are un-parameterized Maps, which scream
warnings when used. Added comment to explain this fact.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@66
PS2, Line 66: return serializer_.options();
> Why is this method empty?
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@74
PS2, Line 74:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@145
PS2, Line 145: obj.serialize(object());
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@151
PS2, Line 151: p
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@46
PS2, Line 46: * by the order that objects are emitted, and thus is consistent
across runs.
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@58
PS2, Line 58: /**
:* Object serializer that generates formatted JSON output.
> If the if condition doesn't fit into a single line, we should add {}.
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@75
PS2, Line 75: void objectList(String name, List
objs);
> I think it's better to not use shorthand name. We don't save that many cha
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@80
PS2, Line 80: stringL
> similarly here, we can call it stringList instead.
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@85
PS2, Line 85: stringL
> same as above
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@32
PS2, Line 32:* Include the source SQL.
:*/
: public ToJsonOptions showSource(boolean flag)
: { showSource_ = flag; return this; }
: /**
:* Include details of the structures created by analyais, such
:* as the output result set.
:*/
: public ToJsonOptions showOutput(boolean flag)
: { showOutput_ = flag; return this; }
: /**
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#4). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with three implementations: * Jackson streaming API to produce standard JSON for export to external systems. * Bespoke JSON serializer optimized to produce compact, repeatable output for testing. This form may be adapted to produce YAML, which is more compact than JSON in order to reduce test file size. * JSONSimple (already an Impala dependency) to create JSON objects. Not super useful, but shows how to use other object-based JSON frameworks if needed. This framework itself is independent of the AST and can also be used to serialize the physical plan as well. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/SerializationError.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 12 files changed, 1,944 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/4 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7902: NumericLiteral fixes, refactoring
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12001 ) Change subject: IMPALA-7902: NumericLiteral fixes, refactoring .. Patch Set 13: Seeing a pre-review test failure; need to investigate. -- To view, visit http://gerrit.cloudera.org:8080/12001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I484600747b2871d3a6fe9153751973af9a8534f2 Gerrit-Change-Number: 12001 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 03:22:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py"
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12096 ) Change subject: IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3572/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 Gerrit-Change-Number: 12096 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 03:18:26 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Improve SCRATCH_ALLOCATION_FAILED error Adds additional context about how much scratch was allocated by the query and the impalad in total. We sometimes see scratch allocation failures because a query was spilling heavily and ate up all the disk. In this case, the high values in the error should provide an additional clue that the volume of spilling is the problem (rather than disks being full for other reasons). Example error after deleting /tmp/impala-scratch: [localhost:21000] default> set mem_limit=150m; select distinct * from tpch_parquet.lineitem limit 5; WARNINGS: Could not create files in any configured scratch directories (--scratch_dirs=/tmp/impala-scratch) on backend 'tarmstrong-box:22000'. 2.00 MB of scratch is currently in use by this Impala Daemon (2.00 MB by this query). See logs for previous errors that may have prevented creating or writing scratch files. Disk I/O error: open() failed for /tmp/impala-scratch/7d473ea7aef26431:c9105f79_3120108e-475b-4616-9825-8bbdb1dc9cc2. The given path doesn't exist. errno=2 Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Reviewed-on: http://gerrit.cloudera.org:8080/12088 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M common/thrift/generate_error_codes.py 3 files changed, 21 insertions(+), 13 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 03:07:17 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 2: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3570/ -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 02:51:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Sat, 15 Dec 2018 01:58:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Reviewed-on: http://gerrit.cloudera.org:8080/11881 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 467 insertions(+), 83 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 14 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1622/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 01:49:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7980: Fix spinning threads because of buggy handling of num unqueued files .
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12097 ) Change subject: IMPALA-7980: Fix spinning threads because of buggy handling of num_unqueued_files_. .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1621/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I133de13238d3d05c510e2ff771d48979125735b1 Gerrit-Change-Number: 12097 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 15 Dec 2018 01:41:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12016 ) Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. IMPALA-7915: Wrap SQL parser to avoid redundant code The FE has several repeated blocks of code to set up the lexer and parser, to parse, and to handle errors. This patch moves this code into a static function that can be used in place of the copies. At the same time, provide a specific ParseException to replace the generic Exception thrown by the parser to allow easier error handling. Some of the uses of the parser assume the return value is Object, others that the value is ParseNode and still others that it is StatementBase. Since the actual return is StatementBase, declares that as the return value of the new static method to clearly state the actual output. Testing: This is just a refactoring. Reran all FE tests to ensure no regressions. Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Reviewed-on: http://gerrit.cloudera.org:8080/12016 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- A fe/src/main/java/org/apache/impala/analysis/Parser.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/catalog/View.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M tests/shell/test_shell_interactive.py 9 files changed, 95 insertions(+), 62 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12016 ) Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Sat, 15 Dec 2018 01:38:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py"
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12096 ) Change subject: IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1620/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 Gerrit-Change-Number: 12096 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 01:33:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7980: Fix spinning threads because of buggy handling of num unqueued files .
Philip Zeyliger has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12097 Change subject: IMPALA-7980: Fix spinning threads because of buggy handling of num_unqueued_files_. .. IMPALA-7980: Fix spinning threads because of buggy handling of num_unqueued_files_. *** WIP: I'm still working on the DCHECK() for num_unqueued_files_. It can't be in SetDone(), because others may call Close() on the scan node. Trying where it is now, but tests haven't run... *** When running TPC-DS query 1 on scale factor 10,000 (10TB) on a 140-node cluster with replica_preference=remote, we observed really high system CPU usage for some of the scan nodes: HDFS_SCAN_NODE (id=6):(Total: 59s107ms, non-child: 59s107ms, % non- child: 100.00% - BytesRead: 80.50 MB (84408563) - ScannerThreadsSysTime: 36m17s Using 36 minutes of system time in only 1 minute of wall-clock time required ~30 threads to be spinning in the kernel. We were able to use perf to finda lot of usage of futex_wait() and pthread_cond_wait(). Eventually, we figured out that ScannerThreads, once started, loop forever looking for work. The case that there is no work is supposed to be rare, and the scanner threads are supposed to exit based on num_unqueued_files_ being 0, but, in some cases, that counter isn't appropriately decremented. The reproduction is any query that uses runtime filters to filter out entire files. Something like: set RUNTIME_FILTER_WAIT_TIME_MS=1; select count(*) from customer join customer_address on c_current_addr_sk = ca_address_sk where ca_street_name="DoesNotExist" and c_last_name="DoesNotExist"; triggers this behavior. Interestingly, though this wastes cycles, query results are unaffected. The point fix is to decrement the counter when skipping files. This bug was co-debugged by Todd Lipcon, Joe McDonnell, Philip Zeyliger, and Michael Ho. Change-Id: I133de13238d3d05c510e2ff771d48979125735b1 --- M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters_wait.test M tests/query_test/test_runtime_filters.py 4 files changed, 18 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12097/1 -- To view, visit http://gerrit.cloudera.org:8080/12097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I133de13238d3d05c510e2ff771d48979125735b1 Gerrit-Change-Number: 12097 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java
File fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@108
PS3, Line 108: }.verify("{\n f_1: null,\n \"f 2\": 10,\n \"f-3\":
null,\n \"f.4\": \"abc\"\n}\n");
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c
Gerrit-Change-Number: 12079
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Sat, 15 Dec 2018 01:07:03 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#3). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with two implementations: one that uses JSONSimple (already an Impala dependency) to create JSON objects, another that uses a bespoke JSON serializer optimized to produce compact, repeatable output for testing. The framework allows other serializers to be added later. Perhaps YAML might prove even more compact than JSON. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 9 files changed, 1,283 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/3 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7980: Fix spinning threads because of buggy handling of num unqueued files .
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12097 ) Change subject: IMPALA-7980: Fix spinning threads because of buggy handling of num_unqueued_files_. .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py File tests/query_test/test_runtime_filters.py: http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py@59 PS1, Line 59: s flake8: E501 line too long (92 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py@96 PS1, Line 96: s flake8: E501 line too long (92 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/12097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I133de13238d3d05c510e2ff771d48979125735b1 Gerrit-Change-Number: 12097 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 15 Dec 2018 00:58:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py"
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12096 ) Change subject: IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12096/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/12096/1/bin/start-impala-cluster.py@373 PS1, Line 373: def wait_for_cluster_web(timeout_in_seconds=CLUSTER_WAIT_TIMEOUT_IN_SECONDS): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/12096/1/bin/start-impala-cluster.py@426 PS1, Line 426: def wait_for_cluster_cmdline(timeout_in_seconds=CLUSTER_WAIT_TIMEOUT_IN_SECONDS): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/12096/1/bin/start-impala-cluster.py@492 PS1, Line 492: i flake8: F401 'json' imported but unused -- To view, visit http://gerrit.cloudera.org:8080/12096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 Gerrit-Change-Number: 12096 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 00:52:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py"
Bharath Vissapragada has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12096 Change subject: IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" .. IMPALA-7989: Revert "Remove Python 2.4 workarounds in start-impala-cluster.py" This reverts commit cf5355bd4051c937100eadc8eaadf77d829f18e2. This revert is temporary until we figure out the ImportError problem. Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 --- M bin/start-impala-cluster.py 1 file changed, 40 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/12096/1 -- To view, visit http://gerrit.cloudera.org:8080/12096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ide42cfd994ea6edc1a9dfa39786e2b805c225656 Gerrit-Change-Number: 12096 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada
[Impala-ASF-CR] IMPALA-4555: Make QueryState's status reporting more robust
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12049 ) Change subject: IMPALA-4555: Make QueryState's status reporting more robust .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1619/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971 Gerrit-Change-Number: 12049 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Dec 2018 00:01:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4555: Make QueryState's status reporting more robust
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12049 ) Change subject: IMPALA-4555: Make QueryState's status reporting more robust .. Patch Set 2: Hue polls the HS2 GetLog() endpoint - i.e. ImpalaServer::GetLog(), but the main purpose of that afaik is to see the query progress. -- To view, visit http://gerrit.cloudera.org:8080/12049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971 Gerrit-Change-Number: 12049 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 23:55:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4555: Make QueryState's status reporting more robust
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/12049 )
Change subject: IMPALA-4555: Make QueryState's status reporting more robust
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/coordinator-backend-state.cc@299
PS1, Line 299: report_seq_no <= instance_stats->last_report_seq_no_ ||
instance_stats->done_) {
> Wouldn't this be a problem for intermediate report ? If the coordinator is
You're right that this approach may cause non-idempotent parts of the report to
be applied twice (at the moment, only the error log).
As discussed offline, only bumping the seq no on the executor side if the
report was successful doesn't work - since we'll wait 5s+ before sending the
new report, it will contain new info that we do want to have applied.
I also looked into the idea of just deferring sending the error log until the
final message. Currently, the error log is only exposed by the beeswax call
get_log(). I don't know if this is commonly called by users in the middle of
query execution (the shell calls it, but only at the end of the query), but
going forward we probably want to expose this in more ways (eg. as part of the
runtime profile), and I think it's a better user experience to make the errors
accessible as soon as possible, for example to help identify issues with
queries that are very long running or appear to hang. Further, there are other
non-idempotent things that we may want to include (eg. it would be nice to have
the dml stats incrementally updated instead of just at the end), so for these
reasons I think its worth trying to come up with a solution for this now.
The solution I put together separates the non-idempotent parts of the report
out into a 'stateful' report. If the report rpc fails, each fragment instance
stores the stateful report it had generated, and adds it to the next report
associated with its original sequence number. This ensures that the coordinator
will only apply stateful portions of the report for sequence numbers it hasn't
seen yet. Once a report is successful, all the stored stateful reports are
cleared.
Downsides to this approach vs. just sending the error log with the final report:
- Copying the stateful report into the fragment instance's state takes time and
adds memory usage and sending multiple stateful reports increases the size of
the report rpc and puts more load on the coordinator. These issues will only
occur if a report rpc actually fails, which is hopefully rare, but if things
are already failing this may bog the system down even more.
- Additional code complexity.
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/fragment-instance-state.h@99
PS1, Line 99: void SetFinalReportSent() { final_report_sent_ = true; }
> May help to add comments on the expected caller and on when this is expecte
Done
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.h@417
PS1, Line 417: (bool final
> Comment on the input parameter.
Done
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc@58
PS1, Line 58: DEFINE_int32(status_report_max_failures, 3,
: "Max number of consecutive failed status reports to allow
before cancelling");
> Actually, max_retries seems to be safer in the sense that it guarantees a m
Done
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc@324
PS1, Line 324: Try to send the RPC 3 times before failing. Sleep for 100ms
between retries.
> May need to be updated.
Done
http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc@368
PS1, Line 368: fis_map_[id]->runtime_state()->ClearUnreportedErrors();
> There is a race here: we may be clearing newly added errors we added after
Done
--
To view, visit http://gerrit.cloudera.org:8080/12049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971
Gerrit-Change-Number: 12049
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Fri, 14 Dec 2018 23:38:08 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4555: Make QueryState's status reporting more robust
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12049 to look at the new patch set (#2). Change subject: IMPALA-4555: Make QueryState's status reporting more robust .. IMPALA-4555: Make QueryState's status reporting more robust QueryState periodically collects runtime profiles from all of its fragment instances and sends them to the coordinator. Previously, each time this happens, if the rpc fails, QueryState will retry twice after a configurable timeout and then cancel the fragment instances under the assumption that the coordinator no longer exists. We've found in real clusters that this logic is too sensitive to failed rpcs and can result in fragment instances being cancelled even in cases where the coordinator is still running. This patch makes a few improvements to this logic: - When an intermediate report is generated, if the first attempt to send it to the coordinator fails, the report is discarded. Only the final report, when all of the fragments instances have completed, is retried. - Exponential backoff is used, both for the time between generating intermediate reports (controlled by FLAG_status_report_interval_ms) and for the time between retries for the final report (controlled by FLAG_report_status_retry_interval_ms) such that for a period between retries of 't', on try 'n' the actual timeout will be t * n. Testing: - Added a test which results in a large number of failed intermediate status reports but still succeeds. Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M common/protobuf/control_service.proto M tests/custom_cluster/test_rpc_timeout.py 7 files changed, 139 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/12049/2 -- To view, visit http://gerrit.cloudera.org:8080/12049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971 Gerrit-Change-Number: 12049 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 22:54:27 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 23:11:01 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3571/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 23:11:02 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 2: Code-Review+2 > tmp-file-mgr-test exercises the code paths. It checks the error > code only, not the full message (which is why I didn't have to > update it). Thanks for the clarification. -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 23:03:36 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 2: tmp-file-mgr-test exercises the code paths. It checks the error code only, not the full message (which is why I didn't have to update it). -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 23:02:43 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3570/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 22:54:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 2: To what extent is this the Jackson streaming API? https://github.com/FasterXML/jackson-docs/wiki/JacksonStreamingApi -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 22:43:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7694: Add host resource usage metrics to profile
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/12069 )
Change subject: IMPALA-7694: Add host resource usage metrics to profile
..
Patch Set 7:
(22 comments)
Thanks! I took a first pass.
If I'm reading it correctly, this implementation will always read /proc/stat
every 500ms, even if nobody has ever requested that data. Do we want to try to
short-circuit it?
I personally found the inheritance (two types of TimeSeriesCounter) kind of
awkward, but that's probably just me. The other variant is to explicitly say as
an argument whether a TimeSeries is sampling or not (e.g., via "number of
samples" or something), but I don't think it matters in the grand scheme of
things.
I found the units (0-255) awkward. I think basis points or percentage points
would be more natural, but I'm flexible.
Thanks!
http://gerrit.cloudera.org:8080/#/c/12069/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12069/7//COMMIT_MSG@7
PS7, Line 7: IMPALA-7694: Add host resource usage metrics to profile
Could you mention that you added a mechanism for periodic counter updates?
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/runtime/exec-env.cc@463
PS7, Line 463: std::bind(::CaptureSystemStateSnapshot,
system_state_info_.get());
I read in effective C++ that I should prefer lambdas to std::bind. Well, I
really only read the table of contents. I assume it totally doesn't matter
here, so as long as we're consistent, I'm good with it.
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/runtime/query-state.cc@296
PS7, Line 296: host_profile_->ClearChunkedTimeSeriesCounters();
Do you know what protects this being a race where you're clearing something
that got added after 293?
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/service/impala-server.cc@1051
PS7, Line 1051: if (rand() < trace_ratio * (RAND_MAX + 1L))
query_ctx->__set_trace_resource_usage(true);
I think rand() isn't thread safe. (And do we initialize it with srand()
somewhere?) On some tracing code I'm working on, I tried:
+ double p = query_ctx->client_request.query_options.tracing_probability;
+ if (p > 0) {
+std::mt19937_64 rng;
+std::uniform_real_distribution unif(0, 1);
+if (unif(rng) < p) GetThreadDebugInfo()->SetTracingEnabled(true);
+ }
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/pretty-printer.h
File be/src/util/pretty-printer.h:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/pretty-printer.h@139
PS7, Line 139: case TUnit::BYTE_FRACTION: {
What is a BYTE_FRACTION?
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile-counters.h@453
PS7, Line 453: class RuntimeProfile::ChunkedTimeSeriesCounter
: : public RuntimeProfile::TimeSeriesCounter {
The inheritance in this file is getting complicated. Could you add a diagram of
the tree somewhere in the headers and indicate what the distinctions between
them all are?
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile-counters.h@474
PS7, Line 474:
: virtual const int64_t* GetSamples(
: int* num_samples, int* period, SpinLock** lock = nullptr)
const override;
Even though this is private, it seems like we should document what lock does
here.
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile.h@386
PS7, Line 386: const std::string& name, TUnit::type unit,
DerivedCounterFunction sample_fn);
What makes sample_fn a "DerivedCounterFunction"? i.e., Derived is unclear to me.
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile.cc@779
PS7, Line 779: // SampliSamplingTimeSeriesCounter.
typo
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile.cc@788
PS7, Line 788: stream << endl;
Consider printing something here that indicates that we actually have more
samples in the thrift profile.
(I think it would also be reasonable to just print everything.)
http://gerrit.cloudera.org:8080/#/c/12069/7/be/src/util/runtime-profile.cc@1238
PS7, Line 1238: DCHECK(period_ == period);
Given that this is a per-process flag, this could actually be false. Do we want
to reject it outright or just
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1618/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:59:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 22:05:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3569/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 22:05:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12016 ) Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3568/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:49:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12016 ) Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:49:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12016 ) Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:48:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12016 ) Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1617/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:47:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 2:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java
File
fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@26
PS2, Line 26:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@28
PS2, Line 28:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@51
PS2, Line 51: if (obj != null)
: obj.serialize(this);
: else if (!options().elide())
: scalar(name, null);
nit:
if (obj != null) obj.serialize(this);
else if (!options().elide()) scalar(name, null);
Braces can be optional if it's in a single line.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@35
PS2, Line 35:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@45
PS2, Line 45: @SuppressWarnings("unchecked")
I may miss something obvious, but I don't see anything that requires
suppressing a warning here and anywhere else in this file.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@66
PS2, Line 66: // TODO Auto-generated method stub
Why is this method empty?
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@74
PS2, Line 74:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@145
PS2, Line 145:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@151
PS2, Line 151:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@46
PS2, Line 46:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@58
PS2, Line 58: if (value != null || !options().elide())
: formatter_.builder_.quotedField(level_, name, value);
If the if condition doesn't fit into a single line, we should add {}.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@75
PS2, Line 75: void objList(String name, List
objs);
I think it's better to not use shorthand name. We don't save that many chars
anyway. We can call it objectList.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@80
PS2, Line 80: strList
similarly here, we can call it stringList instead.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@85
PS2, Line 85: strList
same as above
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@25
PS2, Line 25:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@32
PS2, Line 32: /**
:* Include the source SQL.
:*/
: public ToJsonOptions showSource(boolean flag) { showSource_ =
flag; return this; }
: /**
:* Include details of the structures created by analyais, such
:* as the output result set.
:*/
: public ToJsonOptions showOutput(boolean flag) { showOutput_ =
flag; return
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. Patch Set 13: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 21:39:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 12: Code-Review+2 Thanks for the fixes. This LGTM. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:33:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public StatementBase statement() { return stmt_; }
> I don't actually mean adding Preconditions.checkNotNull but Preconditions.c
Got it, makes sense. Adjusted accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 12
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Fri, 14 Dec 2018 21:24:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#12). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 467 insertions(+), 83 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/12 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1616/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:17:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7915: Wrap SQL parser to avoid redundant code
Hello Bharath Vissapragada, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12016 to look at the new patch set (#4). Change subject: IMPALA-7915: Wrap SQL parser to avoid redundant code .. IMPALA-7915: Wrap SQL parser to avoid redundant code The FE has several repeated blocks of code to set up the lexer and parser, to parse, and to handle errors. This patch moves this code into a static function that can be used in place of the copies. At the same time, provide a specific ParseException to replace the generic Exception thrown by the parser to allow easier error handling. Some of the uses of the parser assume the return value is Object, others that the value is ParseNode and still others that it is StatementBase. Since the actual return is StatementBase, declares that as the return value of the new static method to clearly state the actual output. Testing: This is just a refactoring. Reran all FE tests to ensure no regressions. Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e --- A fe/src/main/java/org/apache/impala/analysis/Parser.java M fe/src/main/java/org/apache/impala/catalog/Type.java M fe/src/main/java/org/apache/impala/catalog/View.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M tests/shell/test_shell_interactive.py 9 files changed, 95 insertions(+), 62 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/12016/4 -- To view, visit http://gerrit.cloudera.org:8080/12016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I174c59d38542ff311c6c3dc10cf3ad4e40f8b30e Gerrit-Change-Number: 12016 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. Patch Set 13: Thanks much for making the changes: the code is now much simpler and easier to read for newbies like me. +1 (non-binding) on the Java side. -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 21:12:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public StatementBase statement() { return stmt_; }
> Actually, if you look inside the checkNull assertion, it simply throws an N
I don't actually mean adding Preconditions.checkNotNull but
Preconditions.checkState instead. I always see Preconditions as a check for
programming error. So, if the code hits Preconditions, that means there's a bug
in the code. With a standard NPE, we can't really sure if the state is valid or
invalid.
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Fri, 14 Dec 2018 20:59:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7902: NumericLiteral fixes, refactoring
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12001 ) Change subject: IMPALA-7902: NumericLiteral fixes, refactoring .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1615/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I484600747b2871d3a6fe9153751973af9a8534f2 Gerrit-Change-Number: 12001 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 20:55:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1614/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 20:51:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 11:
(6 comments)
Thanks for the reviews. Addressed comments and rebased on master.
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public StatementBase statement() { return stmt_; }
> We need a Preconditions to check if the statement has been analyzed or else
Actually, if you look inside the checkNull assertion, it simply throws an NPE
if the object is null. The key value of that check is to clearly state in the
code that something is not supposed to be null. In a more complex method, it
can check invariants at the top to avoid tracking down bigs inside a complex
bit of code.
Here, the code is a one-liner: if the analysis result is null, Java will throw
an NPE exactly as if the Preconditions.checkNotNull() were called. And, someone
looking at the stack should have pretty good idea what is wrong.
So, the question then becomes, does the Preconditions check add value beyond
what the code itself already checks?
I think the answer here is that the message associated with the Preconditions
can help a developer catch errors when learning now to use the fixture. So,
added Preconditions here and elsewhere with messages to explain correct usage.
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@75
PS10, Line 75: @Override
> Similarly, add Preconditions to check if the statement has been analyzed.
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@78
PS10, Line 78: turn analyzer_;
> nit: we can probably join L78 with L77
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@90
PS10, Line 90: for (ExprRewriteRule r : rules) {
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@160
PS10, Line 160:
> line too long (106 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@174
PS10, Line 174: }
> line too long (106 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Fri, 14 Dec 2018 20:44:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#11). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 467 insertions(+), 82 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/11 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7844: HAVING clause cannot support ordinals
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11955 ) Change subject: IMPALA-7844: HAVING clause cannot support ordinals .. Patch Set 6: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1613/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 14 Dec 2018 20:24:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7902: NumericLiteral fixes, refactoring
Hello Bharath Vissapragada, Fredy Wijaya, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12001 to look at the new patch set (#13). Change subject: IMPALA-7902: NumericLiteral fixes, refactoring .. IMPALA-7902: NumericLiteral fixes, refactoring The work to clean up the rewriter logic must start with a stable AST, which must start with sprucing up some issues with the leaf nodes. This CR tackles the NumericLiteral used to hold numbers. IMPALA-7896: Literals should not need explicit analyze step Partial fix: removes the need to analyze a numeric literal: analyze() is a no-op. This eliminates the need to do a "fake" analysis with a null analyzer: numeric literals are now created analyzed. This is useful because the catalog module creates numeric literals outside of a query (and outside of an analyzer.) A literal is immutable except for type. Modified the constructor to set the type and cost, then mark the node as analyzed. A later call to analyze() has nothing to do. Code that created and dummy-analyzed numeric literals changed to use static create() methods resulting in simpler literal creation, and eliminates the special "analyzer == null" checks in analyze(). IMPALA-7886: NumericLiteral constructor fails to round values to Decimal type IMPALA-7887: NumericLiteral fails to detect numeric overflow IMPALA-7888: Incorrect NumericLiteral overflow checks for FLOAT, DOUBLE IMPALA-7891: Analyzer does not detect numeric overflow in CAST IMPALA-7894: Parser does not catch double overflow These are all caused by the somewhat cluttered state of the numeric range check code after years of incremental changes. This patch centralizes all checks into a series of constants and methods for uniformity. All values are set in the constructor which now checks that the value is legal for the type. Cast operations verify that the cast is valid. Multiple semi-parallel versions of the same logic is replaced by calls to a single implementation. The numeric checks now follow the SQL standard which says that implementations should fail if a cast would trucate the most significant digits, but round when truncating the least significant. IMPALA-7865: Repeated type widening of arithmetic expressions Partial fix. Replaces the "is explicit cast" flag in the numeric literal with the explicit type. This allows reseting an implicit type back to the explciit type if an arithmetic expression is analyzed multiple times. A later patch will feed this type information into the type inference mechanism to complete the fix. Finally, adds a set of new exceptions that begin to unify error reporting. These handle casts (SqlCastException), value validation (InvalidValueException) and unsupported features (UnsupportedFeatureException.) These all derive from AnalysisException for backward compatibility. Tests use the new exceptions to check for expected errors rather than parsing text strings (which tend to change.) Testing: * Added unit tests just for numeric literals. Refactored code to simplify the tests. * Added a test case for the obscure case in Decimal V1 of an implicit cast overflow. * The depth-check tests needed one extra level of nesting to trigger failure. * Ran all FE tests. Change-Id: I484600747b2871d3a6fe9153751973af9a8534f2 --- M be/src/exprs/expr-test.cc M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java M fe/src/main/java/org/apache/impala/catalog/ScalarType.java A fe/src/main/java/org/apache/impala/common/InvalidValueException.java A fe/src/main/java/org/apache/impala/common/SqlCastException.java A fe/src/main/java/org/apache/impala/common/UnsupportedFeatureException.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java D fe/src/test/java/org/apache/impala/analysis/ExprTest.java A fe/src/test/java/org/apache/impala/analysis/LiteralExprTest.java A fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java M fe/src/test/java/org/apache/impala/catalog/HdfsPartitionTest.java 20 files changed, 1,415 insertions(+), 286 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12001/13 -- To view, visit http://gerrit.cloudera.org:8080/12001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#2). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with two implementations: one that uses JSONSimple (already an Impala dependency) to create JSON objects, another that uses a bespoke JSON serializer optimized to produce compact, repeatable output for testing. The framework allows other serializers to be added later. Perhaps YAML might prove even more compact than JSON. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 9 files changed, 1,199 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/2 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1612/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 20:13:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 4: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 20:06:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7844: HAVING clause cannot support ordinals
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11955 ) Change subject: IMPALA-7844: HAVING clause cannot support ordinals .. Patch Set 5: (3 comments) Thanks for the additional comments. Addressed the suggestions and rebased on master. http://gerrit.cloudera.org:8080/#/c/11955/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/11955/5/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@351 PS5, Line 351: do not > don't think it is correct now that we disabled it by default. Clarify that Added note about present, intended future support http://gerrit.cloudera.org:8080/#/c/11955/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11955/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1074 PS4, Line 1074: } > Sorry, I meant negative. Done http://gerrit.cloudera.org:8080/#/c/11955/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11955/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1108 PS5, Line 1108: > nit: extra space Done -- To view, visit http://gerrit.cloudera.org:8080/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 14 Dec 2018 20:04:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7844: HAVING clause cannot support ordinals
Hello Bharath Vissapragada, Greg Rahn, Zoltan Borok-Nagy, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11955 to look at the new patch set (#6). Change subject: IMPALA-7844: HAVING clause cannot support ordinals .. IMPALA-7844: HAVING clause cannot support ordinals The SELECT statement has two clauses that take lists of columns and/or aliases: ORDER BY and GROUP BY. Each element is a alias, a table.column reference or a number, which represents the ordinal number of a column. Thus, "GROUP BY a, 2, c" is unambiguous. SELECT also has a number of predicate clauses: WHERE and HAVING. In these, aliases are possible (though seldom suppored), but ordinals are ambiguous: is "WHERE 1 = 2" a reference to two constants, two columns by ordinal, or a combination? No SQL dialect supports ordinals in WHERE or HAVING for this reason. Impala seems to have adopted a rather odd convention: if the HAVING predicate has only one element (no operators), then that one element can be an ordinal or alias. Thus "HAVING a" and "HAVING 1" are valid, only if alias a or the column at ordinal 1 are BOOLEAN. However, "HAVING a = 1" and "HAVING 1 = 2" are not valid. This is unusual because HAVING is normally a predicate: "HAVING a = 10". This fix prepares to remove the attempt to support ordinals in the HAVING clause, after which Impala will treat HAVING like WHERE, rather than trying to treat it like ORDER BY or GROUP BY. Review comments suggest that such a breaking change (even for such a non-standard, undocumented, odd feature) should occur only in a major version. So, for now, the no-ordinal support can be enabled via a constant, but is turned off by default. (Perhaps a later patch can add a session or runtime option instead of the constant.) The fix retains the limited form of alias support. Refactored the "ordinal or alias" code to make alias resolution optional. Reworded a few error messages for greater clarity. Testing: * Refactored AnalyzeStmtsTest to split apart the alias and ordinals cases for easier debugging. * Tests can validate either the current HAVING ordinal behavior or the proposed new behavior. Test path is controlled by the constant described above. Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java 5 files changed, 331 insertions(+), 142 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/11955/6 -- To view, visit http://gerrit.cloudera.org:8080/11955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic2b9f9e8c60fe2b25e20c57c2ffc31d8e59d5861 Gerrit-Change-Number: 11955 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1610/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 20:01:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1611/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 19:59:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1609/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 19:47:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 ) Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. Patch Set 3: (2 comments) Addressed review comments. Rebased on master. http://gerrit.cloudera.org:8080/#/c/12009/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/12009/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@126 PS1, Line 126: Token t = hiveLexer.nextToken(); > Actually forget about this, I missed the try/catch. This code is fine. Thanks. Actually, this is the original code, just moved into a separate function for testing. http://gerrit.cloudera.org:8080/#/c/12009/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@149 PS1, Line 149: // Impala's scanner recognizes the ".123" portion of "db.123e45" as a decimal, : // so while the quoting is not necessary for the given identifier itself, the quotes : // are needed if this identifier will be preceded by a ".". > This is no longer an issue in Impala: https://gerrit.cloudera.org/c/11927/. This comment is original, just moved. Thanks for pointing out the correction. -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 19:54:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Hello Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12009 to look at the new patch set (#4). Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. IMPALA-7905: Hive keywords not quoted for identifiers Impala often generates SQL for statements in the toSql() call, often used during testing or when writing the query plan. Impala keywords such as "create", when used as identifiers, must be quoted: SELECT `select`, `from` FROM `order` ... The code in ToSqlUtils.getIdentSql() also quotes the identifier if it is a Hive keyword. But, that code contained a flaw: it uses the Hive lexer to detect a keyword, but the lexer expects a case-insentisitive input. We provide a case sensitive input. As a result, "MONTH" is caught as a Hive keyword and quoted, but "month" is not. This patch fixes that flaw. Testing: * Refactored getIdentSql() easier testing. * Added a tests to the recently added ToSqlUtilsTest for this case and several others. * Making this change caused the columns `month`, `year`, and `key` to be quoted when before they were not. Updated many tests as a result. * Added a new identSql() function, for use in tests, to match the quoting that Impala uses, and to handle the wildcard, and multi-part names. Used this in ToSqlTest to handle the quoted names. * Reran all FE tests. Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 --- M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlUtilsTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test M testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-stats-agg.test M testdata/workloads/functional-planner/queries/PlannerTest/partition-key-scans.test M testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test M testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test 31 files changed, 719 insertions(+), 495 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/12009/4 -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7963: Don't underflow uint64 t when time goes backwards
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12091 ) Change subject: IMPALA-7963: Don't underflow uint64_t when time goes backwards .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12091 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifa2abbbabe340b3c5f1dbec613219bc964f15747 Gerrit-Change-Number: 12091 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 19:44:40 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12092 ) Change subject: Symbolize stacktraces in debug builds. .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 19:42:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7963: Don't underflow uint64 t when time goes backwards
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12091 ) Change subject: IMPALA-7963: Don't underflow uint64_t when time goes backwards .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1608/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12091 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifa2abbbabe340b3c5f1dbec613219bc964f15747 Gerrit-Change-Number: 12091 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 19:37:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement
..
Patch Set 13:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java:
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@56
PS12, Line 56:
> Nit: less cluttered to put enums, nested classes at the top of the class, f
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@122
PS12, Line 122: protected Action getAction() { return action_; }
> Nit: include "protected" keyword. It is the default, yes, but convenient fo
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@141
PS12, Line 141: if (action_.isRefresh()) {
> action_.isRefresh() -- use method, not private field
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@182
PS12, Line 182: throw new IllegalStateException("Invalid reset metadata
action: " + action_);
> Nit: This is kind of overkill. Simply throw IllegalStateException("Invalid.
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@190
PS12, Line 190: case REFRESH_AUTHORIZATION:
> action_.isRefresh()
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@191
PS12, Line 191: result.append("REFRESH AUTHORIZATION");
> Actually, this whole block can be simpler:
Originally I was following the old style, but I agree I don't think there's any
point saving few words with more complicated if/else/switch. Done.
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@204
PS12, Line 204: result.append("INVALIDATE METADATA");
> Nit: .append(" ").append(partitionSpec...
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@207
PS12, Line 207: result.append("INVALIDATE METADATA
").append(tableName_.toSql());
> Nit: see above
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@216
PS12, Line 216: TResetMetadataRequest params = new TResetMetadataRequest();
> Nit: result.append("INVALIDATE METADATA").append(" ") --> result.append("IN
Good catch! Done.
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@219
PS12, Line 219: params.setTable_name(new TTableName(tableName_.getDb(),
tableName_.getTbl()));
> Nit: see above
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@228
PS12, Line 228: }
> Use method
Done
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3458
PS12, Line 3458: catalog_.getDeleteLog().addRemovedObject(obj);
> addRemovedObject? Does this really add to the catalog an object that was re
Yup, that's what addRemovedObject means. I'd rather not rename anything in the
existing catalog code for now.
No, deletion is handled differently in the catalog, hence the need to have
getDeleteLog().
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java@265
PS12, Line 265: refreshPrivilegesInCatalog(catalog, resetVersions,
sentryRole.getRoleName(), role,
> The arguments are getting complex. And, it is awkward to return values via
It's a bit trickier since the whole thing runs in a thread so there really
isn't a good way to use a return type unless we modify the whole thing to
return a Future, but I think that's a better task for another CR. However, I do
agree that the number of arguments are getting unwieldy. In the next PS, I
introduced a private class AuthorizationCatalog to capture the added/removed
catalog objects. Let me know what you think.
http://gerrit.cloudera.org:8080/#/c/11888/12/fe/src/main/java/org/apache/impala/util/SentryProxy.java@305
PS12, Line 305: user, allUsersPrivileges, authzCatalog);
> This one is also getting cluttered.
Done
--
To view, visit
[Impala-ASF-CR] IMPALA-7795: Implement REFRESH AUTHORIZATION statement
Fredy Wijaya has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/11888 ) Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement .. IMPALA-7795: Implement REFRESH AUTHORIZATION statement This patch implements REFRESH AUTHORIZATION statement to explicitly refresh authorization metadata. This statement is useful to force Impala to refresh its authorization metadata when there is an external update to authorization metadata without having to wait for the Sentry polling or call INVALIDATE METADATA. Some tests were updated to use REFRESH AUTHORIZATION instead of INVALIDATE METADATA to make the tests run faster. Syntax: REFRESH AUTHORIZATION (authorization must be enabled to execute this statement) Testing: - Added new FE tests - Added new E2E authorization tests - Ran all FE tests - Ran all E2E authorization tests Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c --- M common/thrift/CatalogService.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/SentryProxy.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/util/SentryProxyTest.java M tests/authorization/test_authorization.py M tests/authorization/test_grant_revoke.py M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 18 files changed, 441 insertions(+), 203 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/11888/13 -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 2: Do we have tests for these code paths? -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 19:27:33 + Gerrit-HasComments: No
[Impala-ASF-CR] Symbolize stacktraces in debug builds.
Philip Zeyliger has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12092 Change subject: Symbolize stacktraces in debug builds. .. Symbolize stacktraces in debug builds. It's convenient to have symbols if debugging debug builds. Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 --- M be/src/common/init.cc 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/12092/1 -- To view, visit http://gerrit.cloudera.org:8080/12092 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I48cc69829e155a56b59b9e5a821c2af83618bb40 Gerrit-Change-Number: 12092 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12081 ) Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. Patch Set 2: Verified+1 Build Successful https://jenkins.impala.io/job/gerrit-docs-auto-test/179/ : Doc tests passed. -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 19:28:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Alex Rodoni has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12081 ) Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. IMPALA-7978: [DOCS] Clarify the memory requirements Separated the memory requirement and the JVM heap requirement for catalogd. Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Reviewed-on: http://gerrit.cloudera.org:8080/12081 Reviewed-by: Alex Rodoni Tested-by: Alex Rodoni --- M docs/shared/impala_common.xml M docs/topics/impala_prereqs.xml 2 files changed, 204 insertions(+), 171 deletions(-) Approvals: Alex Rodoni: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/12081 ) Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 19:27:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7905: Hive keywords not quoted for identifiers
Hello Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12009 to look at the new patch set (#3). Change subject: IMPALA-7905: Hive keywords not quoted for identifiers .. IMPALA-7905: Hive keywords not quoted for identifiers Impala often generates SQL for statements in the toSql() call, often used during testing or when writing the query plan. Impala keywords such as "create", when used as identifiers, must be quoted: SELECT `select`, `from` FROM `order` ... The code in ToSqlUtils.getIdentSql() also quotes the identifier if it is a Hive keyword. But, that code contained a flaw: it uses the Hive lexer to detect a keyword, but the lexer expects a case-insentisitive input. We provide a case sensitive input. As a result, "MONTH" is caught as a Hive keyword and quoted, but "month" is not. This patch fixes that flaw. Testing: * Refactored getIdentSql() easier testing. * Added a tests to the recently added ToSqlUtilsTest for this case and several others. * Making this change caused the columns `month`, `year`, and `key` to be quoted when before they were not. Updated many tests as a result. * Added a new identSql() function, for use in tests, to match the quoting that Impala uses, and to handle the wildcard, and multi-part names. Used this in ToSqlTest to handle the quoted names. * Reran all FE tests. Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 --- M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlUtilsTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test M testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test M testdata/workloads/functional-planner/queries/PlannerTest/joins.test M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-stats-agg.test M testdata/workloads/functional-planner/queries/PlannerTest/partition-key-scans.test M testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test M testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test M testdata/workloads/functional-planner/queries/PlannerTest/union.test 31 files changed, 720 insertions(+), 495 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/12009/3 -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/12081 ) Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. Patch Set 2: Code-Review+2 Editorial format changes that did not affect the content. -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 19:05:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12081 ) Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. Patch Set 2: Build Started https://jenkins.impala.io/job/gerrit-docs-auto-test/179/ Testing docs change - this change appears to modify docs/ and no code. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 19:04:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Hello Bharath Vissapragada, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12081 to look at the new patch set (#2). Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. IMPALA-7978: [DOCS] Clarify the memory requirements Separated the memory requirement and the JVM heap requirement for catalogd. Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 --- M docs/shared/impala_common.xml M docs/topics/impala_prereqs.xml 2 files changed, 204 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12081/2 -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7963: Don't underflow uint64 t when time goes backwards
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12091 Change subject: IMPALA-7963: Don't underflow uint64_t when time goes backwards .. IMPALA-7963: Don't underflow uint64_t when time goes backwards Some tests on Centos 6 have shown small backwards time drifts. In this case, MonotonicStopWatch currently underflows a uint64_t and returns a very large value from RunningTime(). This changes RunningTime() to return 0 when end < start_. Change-Id: Ifa2abbbabe340b3c5f1dbec613219bc964f15747 --- M be/src/util/stopwatch.h 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/12091/1 -- To view, visit http://gerrit.cloudera.org:8080/12091 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ifa2abbbabe340b3c5f1dbec613219bc964f15747 Gerrit-Change-Number: 12091 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell
[Impala-ASF-CR] IMPALA-7978: [DOCS] Clarify the memory requirements
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12081 ) Change subject: IMPALA-7978: [DOCS] Clarify the memory requirements .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4b0d60f0c8d426569386b12f92d8447b535c095 Gerrit-Change-Number: 12081 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Dec 2018 18:57:28 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1607/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 18:50:51 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1606/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 18:42:13 + Gerrit-HasComments: No
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Tim Armstrong has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12088 ) Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Improve SCRATCH_ALLOCATION_FAILED error Adds additional context about how much scratch was allocated by the query and the impalad in total. We sometimes see scratch allocation failures because a query was spilling heavily and ate up all the disk. In this case, the high values in the error should provide an additional clue that the volume of spilling is the problem (rather than disks being full for other reasons). Example error after deleting /tmp/impala-scratch: [localhost:21000] default> set mem_limit=150m; select distinct * from tpch_parquet.lineitem limit 5; WARNINGS: Could not create files in any configured scratch directories (--scratch_dirs=/tmp/impala-scratch) on backend 'tarmstrong-box:22000'. 2.00 MB of scratch is currently in use by this Impala Daemon (2.00 MB by this query). See logs for previous errors that may have prevented creating or writing scratch files. Disk I/O error: open() failed for /tmp/impala-scratch/7d473ea7aef26431:c9105f79_3120108e-475b-4616-9825-8bbdb1dc9cc2. The given path doesn't exist. errno=2 Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 --- M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M common/thrift/generate_error_codes.py 3 files changed, 21 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/12088/2 -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] Improve SCRATCH ALLOCATION FAILED error
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12088 Change subject: Improve SCRATCH_ALLOCATION_FAILED error .. Improve SCRATCH_ALLOCATION_FAILED error Adds additional context about how much scratch was allocated by the query and the impalad in total. We sometimes see scratch allocation failures because a query was spilling heavily and ate up all the disk. In this case, the high values in the error should provide an additional clue that the volume of spilling is the problem (rather than disks being full for other reasons). Example error after deleting /tmp/impala-scratch: [localhost:21000] default> set mem_limit=150m; select distinct * from tpch_parquet.lineitem limit 5; WARNINGS: Could not create files in any configured scratch directories (--scratch_dirs=/tmp/impala-scratch) on backend 'tarmstrong-box:22000'. 2.00 MB of scratch is currently in use by Impala on this backend (2.00 MB by this query). See logs for previous errors that may have prevented creating or writing scratch files. Disk I/O error: open() failed for /tmp/impala-scratch/7d473ea7aef26431:c9105f79_3120108e-475b-4616-9825-8bbdb1dc9cc2. The given path doesn't exist. errno=2 Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 --- M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M common/thrift/generate_error_codes.py 3 files changed, 21 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/12088/1 -- To view, visit http://gerrit.cloudera.org:8080/12088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icbedd586c57ec02e784143927e82b74455f98dc8 Gerrit-Change-Number: 12088 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] Added timeout to run-all-tests
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12086 ) Change subject: Added timeout to run-all-tests .. Patch Set 1: (5 comments) Thanks! This is just below my threshold for asking you to write it in python, but I think it'd be shorter there if you wanted to go that route. If you wanted to avoid writing the loop to wait for a process to finish, you could do: timeout 60 bash -c 'while true; do ps 15796 &> /dev/null || exit 0; sleep 1; done' (Making the pid there a variable is an exercise.) Once that timeout is done, you have to figure out whether or not you need to do the gdb business. http://gerrit.cloudera.org:8080/#/c/12086/1/bin/run-all-tests-timeout-check.sh File bin/run-all-tests-timeout-check.sh: http://gerrit.cloudera.org:8080/#/c/12086/1/bin/run-all-tests-timeout-check.sh@8 PS1, Line 8: echo "Expected timeout value as an argument" add exit 1? http://gerrit.cloudera.org:8080/#/c/12086/1/bin/run-all-tests-timeout-check.sh@22 PS1, Line 22: SECONDS=0 This confused me! SECONDS is a built-in, as you know, but someone who doesn't know, (or even somebody like me who knew and was about to tell you) is super confused because you're treating it like a normal variable not not resetting it. Anyway, I don't think you need "SECONDS=0" here at all, but you need a comment that "$SECONDS is a bash built-in that counts seconds since bash started. http://gerrit.cloudera.org:8080/#/c/12086/1/bin/run-all-tests-timeout-check.sh@24 PS1, Line 24: sleep 60 I think you can reasonably just sleep 1 here and avoid the complication of repeating yourself in lines 28-30. http://gerrit.cloudera.org:8080/#/c/12086/1/bin/run-all-tests-timeout-check.sh@25 PS1, Line 25: [[ -z "$(ps -p $PPID -o pid=)" ]] && exit This is just: ps $PPID &> /dev/null || exit i.e., you can use the exit code of ps to check for a process. http://gerrit.cloudera.org:8080/#/c/12086/1/bin/run-all-tests-timeout-check.sh@45 PS1, Line 45: gdb -ex "set pagination 0" -ex "thread apply all bt" --batch -p $pid > \ I'm surprised we need "set pagination" in addition to --batch. -- To view, visit http://gerrit.cloudera.org:8080/12086 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefd3e16a973709f27aeffc7a15bcab8fcdbb349b Gerrit-Change-Number: 12086 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 18:04:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Added timeout to run-all-tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12086 ) Change subject: Added timeout to run-all-tests .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1605/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12086 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefd3e16a973709f27aeffc7a15bcab8fcdbb349b Gerrit-Change-Number: 12086 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Comment-Date: Fri, 14 Dec 2018 17:28:46 + Gerrit-HasComments: No
[Impala-ASF-CR] Added timeout to run-all-tests
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12086 Change subject: Added timeout to run-all-tests .. Added timeout to run-all-tests Change-Id: Iefd3e16a973709f27aeffc7a15bcab8fcdbb349b --- M bin/impala-config.sh A bin/run-all-tests-timeout-check.sh M bin/run-all-tests.sh 3 files changed, 72 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/12086/1 -- To view, visit http://gerrit.cloudera.org:8080/12086 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iefd3e16a973709f27aeffc7a15bcab8fcdbb349b Gerrit-Change-Number: 12086 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] Update version to 3.2.0-SNAPSHOT
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12053 ) Change subject: Update version to 3.2.0-SNAPSHOT .. Patch Set 2: No, it was my mistake. I shouldn't have done that. I had to manually verify the asf-site commits, and this change seemed fairly harmless and related to the release process, so I guess it was a wrong force of habit. -- To view, visit http://gerrit.cloudera.org:8080/12053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69547de6e768470820930fe05f444df416c5f1de Gerrit-Change-Number: 12053 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 14 Dec 2018 10:39:02 + Gerrit-HasComments: No
