Uses the macros to replace the common queries. #127

allieychen · 2018-03-02T15:13:09Z

Define NUM_ROWS, SUM_START, SUM_END in QueryFormatter, and replaces them in the query to avoid duplicate code.

TESTED:
deploy_and_run_tests.

allieychen · 2018-03-02T15:16:31Z

Advice is needed for line 213 and line 214 in run_tests.py. It is quite ugly to replace TABLE_NAME twice.

coveralls · 2018-03-02T15:20:37Z

Pull Request Test Coverage Report for Build 379

8 of 14 (57.14%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 90.202%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
gcp_variant_transforms/testing/integration/run_tests.py	8	14	57.14%

Files with Coverage Reduction	New Missed Lines	%
gcp_variant_transforms/testing/integration/run_tests.py	1	26.6%

Totals
Change from base Build 374:	-0.1%
Covered Lines:	3084
Relevant Lines:	3419

💛 - Coveralls

arostamianfar · 2018-03-02T18:33:11Z

gcp_variant_transforms/testing/integration/run_tests.py

    - Replaces TABLE_NAME with the table associated for the query.
    """
-    return (' ').join(query).format(TABLE_NAME=self._table_name)
+    return (' ').join(query).format(NUM_ROWS=self.NUM_ROWS,


Consider breaking this up into two helper methods: _replace_macros and _replace_variables.
so, it would be return _replace_variables(_replace_macros(' '.join(query))
you can optionally create meaningful variable names for the intermediate strings.

arostamianfar · 2018-03-02T18:37:25Z

gcp_variant_transforms/testing/integration/run_tests.py

+  Replaces keyword TABLE_NAME and macros in the query.
  """

+  NUM_ROWS = 'SELECT COUNT(0) AS num_rows FROM {TABLE_NAME}'


nit: to make these distinct from the rest of the variables (e.g. TABLE_NAME), what do you think of adding a _QUERY suffix to all of them?

arostamianfar · 2018-03-02T18:47:04Z

gcp_variant_transforms/testing/integration/run_tests.py

+  Replaces keyword TABLE_NAME and macros in the query.
  """

+  NUM_ROWS = 'SELECT COUNT(0) AS num_rows FROM {TABLE_NAME}'


I think it's cleaner to create an enum of these macros. It both provides a natural grouping of these constants and also is easier to loop through them (e.g. adding a new macro is just as easy as adding a new entry in that Enum).
So, it would be something like:

import enum class _QueryMacros(enum.Enum): # nested under the QueryFormatter class NUM_ROWS_QUERY = 'SELECT ...' SUM_START_QUERY = 'SELECT ...' def _replace_macros(query): for macro in _QueryMacros: if macro.name == query: return macro.value else: return query

This also means that you'd no longer need the surrounding {} brackets in the test def, which further distinguishes macros from regular variable names.

Thanks for such a detailed comment! DONE.

I actually got a warning on "for macro in _QueryMacros", says "Expected expected collections.iterable got Type[_QueryMacros] instead. Any idea how to fix this? I have found one way which loops through the members.items, but I don't think it is as readable as current code.

arostamianfar · 2018-03-02T18:50:12Z

gcp_variant_transforms/testing/integration/small_tests/valid_4_0.json

-        "  SUM(start_position) AS sum_start, ",
-        "  SUM(end_position) AS sum_end ",
-        "FROM {TABLE_NAME}"
+        "{NUM_ROWS}"


nit: I think it's cleaner to just put these on one line now that you don't have multiple lines.
e.g.

{ "query": [ "NUM_ROWS_QUERY" ], "expected_result": { "num_rows": 5 } }, { "query": [ "SUM_START_QUERY" ], "expected_result": { "sum_start": 55 } },

Good point!

arostamianfar · 2018-03-02T19:02:12Z

gcp_variant_transforms/testing/integration/run_tests.py


  def format_query(self, query):
-    # type: (List[str]) -> str
+    # type: (list[str]) -> str


Interesting. Is the correct format list instead of List?

I think so, List actually gives a warning.

@bashir2 FYI, looks like we should be using list instead of List

Not really; I mean, I think IntelliJ is smart enough to understand list but the right choice is List according to PEP 484, AFAIK. The reason that you have been getting warnings for List is because you have not imported it from the typing module, check processed_variant.py for an example.

I see. I will correct it.

bashir2 · 2018-03-04T23:54:36Z

gcp_variant_transforms/testing/integration/run_tests.py


  def format_query(self, query):
-    # type: (List[str]) -> str
+    # type: (list[str]) -> str


Not really; I mean, I think IntelliJ is smart enough to understand list but the right choice is List according to PEP 484, AFAIK. The reason that you have been getting warnings for List is because you have not imported it from the typing module, check processed_variant.py for an example.

bashir2 · 2018-03-05T00:09:11Z

gcp_variant_transforms/testing/integration/run_tests.py


    Formatting logic is as follows:
    - Concatenates ``query`` parts into one string.
+    - Replaces macro NUM_ROWS_QUERY/SUM_START_QUERY/SUM_END_QUERY with the


Consider not listing the members of _QueryMacros here such that adding a new query or changing the name of one in _QueryMacros does not an update of this documentation (which will probably not happen making the documentation obsolete in future).

allieychen · 2018-03-05T14:26:53Z

PTAL.

arostamianfar

Looks great! Thanks! Just some minor nits.

arostamianfar · 2018-03-05T17:17:03Z

gcp_variant_transforms/testing/integration/run_tests.py


+  class _QueryMacros(enum.Enum):
+    NUM_ROWS_QUERY = 'SELECT COUNT(0) AS num_rows FROM {TABLE_NAME}'
+    SUM_START_QUERY = ('SELECT SUM(start_position) AS sum_start FROM {'


nit: consider the following format as it's more readable (in general, try to avoid breaking lines as much as possible).

SUM_START_QUERY = ( 'SELECT SUM(start_position) AS sum_start FROM {TABLE_NAME}')

Thanks, done.

arostamianfar · 2018-03-05T17:19:17Z

gcp_variant_transforms/testing/integration/run_tests.py

    - Replaces TABLE_NAME with the table associated for the query.
    """
-    return (' ').join(query).format(TABLE_NAME=self._table_name)
+    return self._replace_variables(self._replace_macros((' ').join(query)))


nit: do you need the brackets around ' '? i.e. does ..._macros(' '.join(query))) work?

arostamianfar · 2018-03-05T17:20:35Z

gcp_variant_transforms/testing/integration/run_tests.py

  """Formats a query.

-  Replaces keyword TABLE_NAME and eventually macros in the query.
+  Replaces macros and variable TABLE_NAME in the query.


nit: by the same token as Bashir's comment, please remove 'table_name' from here as well. (just "Replaces macros and variables in the query").

allieychen · 2018-03-05T18:12:34Z

Thanks. PTAL :)

arostamianfar

LGTM

* Add multiple query support to integration tests googlegenomics#120. update all tests in integration small_tests to support multiple query. update the validate_table in run_tests, add a loop for all test cases in the test file. changed the required keys for the .json file. Remove "validation_query" and "expected_query_result", and add "test_cases". Ran ./deploy_and_run_tests.sh and all integration tests passed. Update the development guide doc (googlegenomics#124) Update the development guide doc. Add IntelliJ IDE setup. Add more details. Added an INFO message for the full command. Tested: Ran manually and checked the new log message. Uses the macros to replace the common queries. (googlegenomics#127) Define NUM_ROWS, SUM_START, SUM_END in QueryFormatter, and replaces them in the query to avoid duplicate code. TESTED: deploy_and_run_tests. Define SchemaDescriptor class. Provides serialization and lookup API for type/mode of schema fields. Tested: unit test

Define NUM_ROWS, SUM_START, SUM_END in QueryFormatter, and replaces them in the query to avoid duplicate code. TESTED: deploy_and_run_tests.

arostamianfar suggested changes Mar 2, 2018

View reviewed changes

arostamianfar reviewed Mar 2, 2018

View reviewed changes

allieychen added 2 commits March 2, 2018 17:12

Uses the macros to replace the common queries.

878e5b6

Use enum for query macros.

663e31d

allieychen force-pushed the query-macros branch from a27a4d0 to 663e31d Compare March 2, 2018 22:13

bashir2 reviewed Mar 5, 2018

View reviewed changes

Use List for type annotation and remove listing members in doc

d3224e3

arostamianfar suggested changes Mar 5, 2018

View reviewed changes

Remove specific variable name in doc and correct code format

aa5bd30

arostamianfar approved these changes Mar 5, 2018

View reviewed changes

Merge branch 'master' into query-macros

653b45b

allieychen merged commit b8a6c8c into googlegenomics:master Mar 5, 2018

allieychen deleted the query-macros branch March 5, 2018 18:26

Uses the macros to replace the common queries. #127

Uses the macros to replace the common queries. #127

Uh oh!

Conversation

allieychen commented Mar 2, 2018

Uh oh!

allieychen commented Mar 2, 2018

Uh oh!

coveralls commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 379

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen commented Mar 5, 2018

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen commented Mar 5, 2018

Uh oh!

arostamianfar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Mar 2, 2018 •

edited

Loading

allieychen Mar 2, 2018 •

edited

Loading