Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/23627 )

Change subject: IMPALA-14553: Run schema eval concurrently
......................................................................


Patch Set 12:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/23627/12/testdata/bin/generate-schema-statements.py
File testdata/bin/generate-schema-statements.py:

http://gerrit.cloudera.org:8080/#/c/23627/12/testdata/bin/generate-schema-statements.py@735
PS12, Line 735:   """section_str should be the contents of a section (i.e. a 
string). If section_str
              :   starts with `, evaluates section_str as a shell command and 
returns the
              :   output. Otherwise returns section_str."""
Please add some comments about the new thread pool method and its implication 
(the need to call unwrap later on).


http://gerrit.cloudera.org:8080/#/c/23627/12/testdata/bin/generate-schema-statements.py@773
PS12, Line 773: []
nit: list()


http://gerrit.cloudera.org:8080/#/c/23627/12/testdata/bin/generate-schema-statements.py@779
PS12, Line 779: f"
Are we safe to use python f string now, or is it better to use older way of 
string format for backward compatibility (older than Python 3.6)?


http://gerrit.cloudera.org:8080/#/c/23627/12/testdata/bin/generate-schema-statements.py@831
PS12, Line 831:   sections = new_sections
Is it possible to unwrap and close the thread pool early here?

  unwrapped_sections = list()
  for s in new_sections:
    unwrapped = dict()
    for k, v in s.items():
      unwrapped[k] = unwrap(v)
    unwrapped_sections.append(unwrapped)
  pool.close()

  sections = unwrapped_sections

By doing that, you can contain L771 - L831 into a single inner function with 
all eval_section already resolved.



--
To view, visit http://gerrit.cloudera.org:8080/23627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a78d05fd6a0005c83561978713237da2dde6af2
Gerrit-Change-Number: 23627
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Jason Fehr <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Riza Suminto <[email protected]>
Gerrit-Comment-Date: Fri, 14 Nov 2025 03:55:05 +0000
Gerrit-HasComments: Yes

Reply via email to