zabetak commented on code in PR #5872:
URL: https://github.com/apache/hive/pull/5872#discussion_r2197941696
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -77,6 +99,24 @@ private HiveHookEvents.HiveHookEventProto
buildIn2Steps(JSONObject json) {
.setExecutionMode(TEZ);
Map<HiveProtoLoggingHook.OtherInfoType, JSONObject> otherInfo = new
HashMap<>();
otherInfo.put(HiveProtoLoggingHook.OtherInfoType.CONF, json);
- return new HiveHookEventProtoPartialBuilder(builder, null, otherInfo,
null, null).build();
+ return new HiveHookEventProtoPartialBuilder(builder, work, otherInfo,
null, stageIdRearrange).build();
+ }
+
+ private ExplainWork getWork() {
+ CompilationOpContext cCtx = new CompilationOpContext();
+ TableScanOperator scanOp = new TableScanOperator(cCtx);
+
+ FetchWork taskWork = new FetchWork(mock(Path.class),
mock(TableDesc.class));
+ taskWork.setSource(scanOp);
+
+ // Create a FetchTask with the FetchWork
Review Comment:
The comment does not add much value; its pretty obvious that we create a
task with some work.
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -77,6 +99,24 @@ private HiveHookEvents.HiveHookEventProto
buildIn2Steps(JSONObject json) {
.setExecutionMode(TEZ);
Map<HiveProtoLoggingHook.OtherInfoType, JSONObject> otherInfo = new
HashMap<>();
otherInfo.put(HiveProtoLoggingHook.OtherInfoType.CONF, json);
- return new HiveHookEventProtoPartialBuilder(builder, null, otherInfo,
null, null).build();
+ return new HiveHookEventProtoPartialBuilder(builder, work, otherInfo,
null, stageIdRearrange).build();
+ }
+
+ private ExplainWork getWork() {
Review Comment:
This is not a getter but rather a factory so it would make more sense to
name it accordingly. For instance:
* createExplainWork
* emptyExplainWork
* emptyWork
* ...
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -49,6 +58,15 @@ public void testEquality() {
assertArrayEquals(event1.toByteArray(), event2.toByteArray());
}
+ @Test
+ public void testOtherInfoQueryPlan() {
+ String stageIdRearrange =
HiveConf.ConfVars.HIVE_STAGE_ID_REARRANGE.defaultStrVal;
+ HiveHookEvents.HiveHookEventProto event =
buildIn2Steps(mock(JSONObject.class), getWork(), stageIdRearrange);
+ // Without HIVE-29012, OTHERINFO will have CONF initially i.e size is 1.
+ // With HIVE-29012, OTHERINFO should have QUERY as well i.e size is 2.
+ assert(event.getOtherInfo(1).hasKey());
Review Comment:
I believe that using a java assertion here was not intentional; please
replace this with a junit assertion.
In addition, consider using `assertEquals` with the full `List` instead of
assertTrue/False. The failure message will be much more descriptive if
something breaks.
Moreover, instead of putting the expectations in a comment (i.e., CONF +
QUERY) just create manually the expected List and base your assertion there.
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -77,6 +99,24 @@ private HiveHookEvents.HiveHookEventProto
buildIn2Steps(JSONObject json) {
.setExecutionMode(TEZ);
Map<HiveProtoLoggingHook.OtherInfoType, JSONObject> otherInfo = new
HashMap<>();
otherInfo.put(HiveProtoLoggingHook.OtherInfoType.CONF, json);
- return new HiveHookEventProtoPartialBuilder(builder, null, otherInfo,
null, null).build();
+ return new HiveHookEventProtoPartialBuilder(builder, work, otherInfo,
null, stageIdRearrange).build();
+ }
+
+ private ExplainWork getWork() {
Review Comment:
The method can also be made static.
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -77,6 +99,24 @@ private HiveHookEvents.HiveHookEventProto
buildIn2Steps(JSONObject json) {
.setExecutionMode(TEZ);
Map<HiveProtoLoggingHook.OtherInfoType, JSONObject> otherInfo = new
HashMap<>();
otherInfo.put(HiveProtoLoggingHook.OtherInfoType.CONF, json);
- return new HiveHookEventProtoPartialBuilder(builder, null, otherInfo,
null, null).build();
+ return new HiveHookEventProtoPartialBuilder(builder, work, otherInfo,
null, stageIdRearrange).build();
+ }
+
+ private ExplainWork getWork() {
+ CompilationOpContext cCtx = new CompilationOpContext();
+ TableScanOperator scanOp = new TableScanOperator(cCtx);
+
+ FetchWork taskWork = new FetchWork(mock(Path.class),
mock(TableDesc.class));
Review Comment:
Do we really need mocking? Path and TableDesc should be easy to create no?
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -66,6 +84,10 @@ private HiveHookEvents.HiveHookEventProto
buildWithOtherInfo(JSONObject json) {
}
private HiveHookEvents.HiveHookEventProto buildIn2Steps(JSONObject json) {
+ return buildIn2Steps(json, null, null);
+ }
Review Comment:
This is not production code so we don't care much about retaining backward
compatibility. Let's just keep the new method.
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -77,6 +99,24 @@ private HiveHookEvents.HiveHookEventProto
buildIn2Steps(JSONObject json) {
.setExecutionMode(TEZ);
Map<HiveProtoLoggingHook.OtherInfoType, JSONObject> otherInfo = new
HashMap<>();
otherInfo.put(HiveProtoLoggingHook.OtherInfoType.CONF, json);
- return new HiveHookEventProtoPartialBuilder(builder, null, otherInfo,
null, null).build();
+ return new HiveHookEventProtoPartialBuilder(builder, work, otherInfo,
null, stageIdRearrange).build();
Review Comment:
Since stageIdRearrange receives only one value during these tests consider
setting directly here.
##########
ql/src/test/org/apache/hadoop/hive/ql/hooks/TestHiveHookEventProtoPartialBuilder.java:
##########
@@ -49,6 +58,15 @@ public void testEquality() {
assertArrayEquals(event1.toByteArray(), event2.toByteArray());
}
+ @Test
+ public void testOtherInfoQueryPlan() {
+ String stageIdRearrange =
HiveConf.ConfVars.HIVE_STAGE_ID_REARRANGE.defaultStrVal;
+ HiveHookEvents.HiveHookEventProto event =
buildIn2Steps(mock(JSONObject.class), getWork(), stageIdRearrange);
Review Comment:
Why mock a JSONObject it's equally simple to do `new JSONObject()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
