Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8166 )
Change subject: IMPALA-5986: Correct set-option logic to recognize digits in names. ...................................................................... Patch Set 1: Code-Review+1 (1 comment) I'm fine with this as is, since it's strictly better, but I think it may make sense to clear up the [A-Z] stuff if it's truly case-insensitive already. http://gerrit.cloudera.org:8080/#/c/8166/1/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/8166/1/tests/common/impala_test_suite.py@103 PS1, Line 103: SET_PATTERN = re.compile(r'\s*set\s*([a-zA-Z_][a-zA-Z0-9_]*)\s*=*', re.I) Since this is a case insensitive RE (re.I), I think you don't need the A-Z here. If we wanted to be extra careful, we could write a test because Thrift exposes the metadata for us like so: >>> import ImpalaInternalService.ttypes >>> options = [ x[2] for x in >>> ImpalaInternalService.ttypes.TQueryOptions.thrift_spec if x ] >>> options ['abort_on_error', 'max_errors', 'disable_codegen', 'batch_size', 'num_nodes', 'max_scan_range_length', 'num_scanner_threads', 'max_io_buffers', 'allow_unsupported_formats', 'default_order_by_limit', 'debug_action', 'mem_limit', 'abort_on_default_limit_exceeded', 'compression_codec', 'hbase_caching', 'hbase_cache_blocks', 'parquet_file_size', 'explain_level', 'sync_ddl', 'request_pool', 'v_cpu_cores', 'reservation_request_timeout', 'disable_cached_reads', 'disable_outermost_topn', 'rm_initial_mem', 'query_timeout_s', 'buffer_pool_limit', 'appx_count_distinct', 'disable_unsafe_spills', 'seq_compression_mode', 'exec_single_node_rows_threshold', 'optimize_partition_key_scans', 'replica_preference', 'schedule_random_replica', 'scan_node_codegen_threshold', 'disable_streaming_preaggregations', 'runtime_filter_mode', 'runtime_bloom_filter_size', 'runtime_filter_wait_time_ms', 'disable_row_runtime_filtering', 'max_num_runtime_filters', 'parquet_annotate_strings_utf8', 'parquet_fallback_schema_resolution', 'mt_dop', 's3_skip_insert_staging', 'runtime_filter_min_size', 'runtime_filter_max_size', 'prefetch_mode', 'strict_mode', 'scratch_limit', 'enable_expr_rewrites', 'decimal_v2', 'parquet_dictionary_filtering', 'parquet_array_resolution', 'parquet_read_statistics', 'default_join_distribution_mode', 'disable_codegen_rows_threshold', 'default_spillable_buffer_size', 'min_spillable_buffer_size', 'max_row_size'] That said, I think the RE now matches what a Thrift identifier per https://thrift.apache.org/docs/idl can be. The docs also say "." can happen, but I don't think you can have a field with a dot in it. -- To view, visit http://gerrit.cloudera.org:8080/8166 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3ba641553ff827dbd4673b9fe7ed7d9d5e83052 Gerrit-Change-Number: 8166 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wood <tw...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Wood <tw...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Sep 2017 17:58:04 +0000 Gerrit-HasComments: Yes
