Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15105 )
Change subject: IMPALA-5444: Asynchronous code generation ...................................................................... Patch Set 43: Code-Review+2 (5 comments) I have a few optional comments about the tests. Great work! http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_async_codegen.py File tests/query_test/test_async_codegen.py: http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_async_codegen.py@83 PS43, Line 83: cls.ImpalaTestMatrix.add_dimension(create_exec_option_dimension( : cluster_sizes=[1], : disable_codegen_options=[False], : batch_sizes=[0], : disable_codegen_rows_threshold_options=[0], : debug_action_options=[cls.DEBUG_ACTION_CODEGEN_FINISH_BEFORE_EXEC_START, : cls.DEBUG_ACTION_CODEGEN_FINISH_DURING_EXEC, : cls.DEBUG_ACTION_EXEC_FINISH_BEFORE_CODEGEN])) maybe it could be simpler to run the query with the 3 different debug actions in test_async_codegen instead of creating an extra dimension http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_async_codegen.py@136 PS43, Line 136: def __find_event_sequence(self, profile): : # The lines corresponding to the events in the event sequence. : events = [] : : # The number of leading whitespace in the lines containing the events, used to : # detect the line after the last event. : indent_len = None : : # Set to true when encountering the header before the first event. This means the : # following lines contain the events. : found_events_start = False : : for line in profile.split('\n'): : if found_events_start: : leading_whitespace = len(line) - len(line.lstrip()) : if indent_len is None: : # This was the first event. We store the indentation of the events. : indent_len = leading_whitespace : elif leading_whitespace < indent_len: : # We've reached the line after the events, stop the iteration. : break : : # If we reach here we are processing a line containing an event. : events.append(self.__extract_event_name(line)) : : elif 'Fragment Instance Lifecycle Event Timeline' in line: : found_events_start = True : : return events : : def __extract_event_name(self, line): : start = line.index('-') + 2 # There is a space after the dash. : end = line.index(':') : return line[start:end] This could be moved to common code, e.g. https://github.com/apache/impala/blob/master/tests/common/test_result_verifier.py We do similar things at other places, e.g. https://github.com/apache/impala/blob/2e07d0c07febf1d1ee9324708d543b792fb45b00/tests/query_test/test_observability.py#L404, but I prefer your version. http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_queries.py File tests/query_test/test_queries.py: http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_queries.py@75 PS43, Line 75: parquet nit: Parquet http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_queries.py@80 PS43, Line 80: beeswax I would prefer hs2, as beeswax may be deprecated in the not too far future. http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_query_mem_limit.py File tests/query_test/test_query_mem_limit.py: http://gerrit.cloudera.org:8080/#/c/15105/43/tests/query_test/test_query_mem_limit.py@30 PS43, Line 30: add_exec_option_dimension, Did something change in this file? -- To view, visit http://gerrit.cloudera.org:8080/15105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7cbfa7c6734dcf03641629429057d6a4194aa6b Gerrit-Change-Number: 15105 Gerrit-PatchSet: 43 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 30 Jun 2020 15:13:37 +0000 Gerrit-HasComments: Yes
