mock flink sql gateway client #59

zqWu · 2023-03-15T08:18:07Z

Description

a mock sql flink client
Resolves #34

receive any statement, save to list
return {"status":"finished"} for any operation_handle

PR Checklist

Tests added
Changelog updated

gliter · 2023-03-15T09:05:13Z

I hope you don't mind. I have converted this into draft. I see that you are still working on it. Please use Ready for review button once this is ready.

zqWu · 2023-03-15T09:39:06Z

test_seeds.py passed
but other tests, fail becase they need get result from "client", which hasn't implemented

test_table_materialization.py
test_source_schema_generation.py

gliter · 2023-03-15T10:50:50Z

tests/functional/adapter/test_seeds.py

+        for i in range(0, len(stats)):
+            assert sql_equivalent(stats[i], expect_sql_sequential[i])
+
+        # run a second time, need clean


Why do we need to test the same thing twice?

just an example in case someone need run operation (like dbt-run) multiple time in one function, i'll remove it

Thanks, with this mock it won't test anything new but with actual Flink instance it will be a valid test.

gliter · 2023-03-15T10:52:38Z

tests/conftest.py

    }
+
+    # we need setup MockFlinkSqlGatewayClient
+    MockFlinkSqlGatewayClient.create_session(


Can we make it to create and use instant instead of using it like singleton?
This will lead to problems in case of running tests in parallel or just not cleaning after in some single test.
Tests by nature should be isolated from each other as much as possible.

you are absolutely right, i'll handle it

gliter · 2023-03-15T10:55:55Z

tests/functional/adapter/test_seeds.py

+            """
+        ]
+        stats = MockFlinkSqlGatewayClient.all_statements()
+        assert len(stats) == len(expect_sql_sequential)


This code could be extracted to separate class like AssertionUtils which we could add as extension to test cases and in tests just call
self.assertSqlEquals(mock.all_statements(), expected_sqls)
very similar how you would use regular asserts in unittests https://docs.python.org/3/library/unittest.html

def test_upper(self): self.assertEqual('foo'.upper(), 'FOO')

gliter · 2023-03-15T10:58:07Z

tests/functional/adapter/test_seeds.py

@@ -46,6 +48,7 @@
 select /** fetch_timeout_ms(10000) */ /** fetch_mode('streaming') */ * from base where id = 10
 """

+
 class TestSeeds():


Could I ask you to move this class to new directory named component so we we will have unit tests in regular directories that correspond to modules its testing, component which will have all the tests with the mock you have created and then e2e for tests with actual Flink intance. What do you think?

gliter · 2023-03-15T10:59:49Z

tests/sqlgateway/mock/gw_router.py

+    @staticmethod
+    def start():
+        httpretty.enable()
+        # DO NOT change register order, httpretty regex match = first match


Please remove copied comment.

gliter · 2023-03-15T11:00:28Z

tests/sqlgateway/mock/mock_client.py

+        return MockFlinkSqlGatewayClient.router.all_statements()
+
+
+def sql_equivalent(s1: str, s2: str) -> bool:


This would land in the assert utils

gliter · 2023-03-15T11:00:52Z

tests/tmp/test_mock_gw.py

+        self.assertEqual(200, r.status_code)
+        print(r.text)
+
+        # session 测试 =========


Please keep comments in English

I hope that's not a problem.

…ments

zqWu · 2023-03-15T15:46:54Z

done

gliter · 2023-03-16T15:57:59Z

tests/component/e2e/test_seeds.py

@@ -0,0 +1,106 @@
+import os


We have got small missunderstanding. This would be just a component test as it is using mock instead of real Flink, so should be in test/component in the future we should create an end-to-end test that will start actual Flink instance during test and it would be kept in test/e2e

gliter · 2023-03-16T16:02:43Z

tests/sqlgateway/mock/mock_client.py

+        return self.router.all_statements()
+
+    @staticmethod
+    def setup(host: str = None, port: str | int = None, session_name: str = None):


the union | operator is as far as I am aware available from 3.10. It has unfortunately still used by very few people https://w3techs.com/technologies/history_details/pl-python/3
in setup.py we are stating this library is using Python 3.7 or newer. Can we get rid of this for now? Maybe lets standardize port as int type.

gliter · 2023-03-16T16:05:39Z

tests/sqlgateway/mock/mock_client.py

+import json
+
+
+class MockFlinkSqlGatewayClient:


I got very confused by this name. I initially thought this is a replacment for flink.sqlgateway.client.FlinkSqlGatewayClient but it is actually a Mock of an SqlGateway and we still use the regular client. So maybe the better name would be just MockFlinkSqlGateway?

that is a mock sqlgateway

gliter · 2023-03-16T16:07:50Z

tests/sqlgateway/mock/mock_client.py

+        self.router = GwRouter(test_config)
+        self.router.start()
+        # create session
+        r = requests.post(f"{host_port}/v1/sessions", json.dumps({"sessionName": f"{session_name}"}))


This seems like not actually used. It will be the real flink.sqlgateway.client.FlinkSqlGatewayClient that will be opening the session anyway.

gliter · 2023-03-16T16:08:05Z

tests/sqlgateway/mock/mock_client.py

+        session_handle = r.json()['sessionHandle']
+        session = SqlGatewaySession(SqlGatewayConfig(host, port, session_name), session_handle)
+        self.session = session
+        return session


This return is ignored by rest of the code.

gliter · 2023-03-16T16:08:18Z

tests/sqlgateway/mock/mock_client.py

+        self.session = session
+        return session
+
+    def execute_statement(self, sql: str) -> SqlGatewayOperation:


This is not used.

gliter · 2023-03-16T16:15:28Z

@zqWu Sorry for the delay. Had finally a chance to actually run properly your PR. This looks really great! Thank you for that work. I have left some comments. Looked like you initially wanted to substitute flink.sqlgateway.client.FlinkSqlGatewayClient and later changed your mind to use http mock. Am I right? There are leftovers from your initial idea.

Thanks for changing it to use instance. I am not familiar with that library, would it work if we would create few instances (but using different port each time) in parallel? This is problem for another time anyway.

Also in the future I guess we will need to add more complex behaviors like making it return error or some specific response.

Please check my comments and then I can merge it.

zqWu · 2023-03-17T05:55:43Z

thanks for review, i'll handle the problems above

zqWu · 2023-03-17T06:20:28Z

i have implemented a mock sqlgateway, with some test code.
still in progress

i think, it will be useful for some test ( not stream and some flink specific sql)

for real e2e test when not use default_catalog, it requires:

gateway, flink, and other like minio, hms, mysql, kafka (even a nginx if you want to see all the sql)
maual init a session, and create a catalog in this session ( or some other init like db/table for test )
set this session in ~/.dbt/flink-session.yaml

these works seems heavy to repeat
with mock gateway can sav a lot effort
it can run in debug mode in pycharm, pretty friend for beginners like me

gliter · 2023-03-17T10:00:50Z

Should be doable using testcontainers. This is something to be left for the future.

gliter · 2023-03-20T06:50:27Z

tests/component/mock_gw/gw_backend.py

+        else:
+            raise "NOT support"
+
+    def _execute_by_sqlite(self, sql) -> list[Any]:


Return type should be List[Any] imported from typing

gliter · 2023-03-20T06:51:04Z

tests/component/mock_gw/rest_api_data/data_provider.py

+        return [x.replace(pattern, chars) for x in r] if pattern else r
+
+    @staticmethod
+    def _add_data_node(res_list, kind, fields: str | List[Any]):


Union of types is not supported in Python < 3.10

gliter · 2023-03-20T06:53:43Z

The file paths are unnecessary nested. Component tests should just be in tests/component instead of test/component/mock_gw/tests.

zqWu · 2023-03-21T03:33:12Z

my branch for this pr is messy, gonna close this and promot another

zqWu force-pushed the mock-sqlgateway-2 branch from 39693f2 to b154a33 Compare March 15, 2023 08:23

mock flink sql gateway client

f8b5416

zqWu force-pushed the mock-sqlgateway-2 branch from b154a33 to f8b5416 Compare March 15, 2023 08:30

gliter marked this pull request as draft March 15, 2023 09:04

use MockFlinkSqlGatewayClient test test_seeds.py, pass

7ad895a

zqWu force-pushed the mock-sqlgateway-2 branch from 41c4037 to 7ad895a Compare March 15, 2023 10:33

zqWu marked this pull request as ready for review March 15, 2023 10:35

gliter reviewed Mar 15, 2023

View reviewed changes

zqWu added 3 commits March 15, 2023 22:17

fix: MockSqlGatewayClient static -> instance and some trivial improve…

f72dc7f

…ments

refactor: add component dir for e2e tests

224c47c

refactor: better error message for test

5142642

zqWu added 2 commits March 16, 2023 10:59

better code, test for AssertUtils

86425ea

fix: mock operation handle increment

290713e

gliter reviewed Mar 16, 2023

View reviewed changes

refactor: component MockSqlGateway

4fb8400

zqWu added 2 commits March 17, 2023 18:03

rename

40284bf

pre-commit check

aa544f7

gliter reviewed Mar 20, 2023

View reviewed changes

zqWu added 6 commits March 20, 2023 16:47

fix: gwBackend ignore /* _comments_ */, move out tests/ in components

a1288f4

fix: sql need lower before handled

353f6b6

remove union of types (3.10+), return type list[Any] => List[Any]

4ec5cbf

mock geteway support 'show table like '

288215a

fix: sql remove comment for sqlite execute

0c4c34f

add test case for no comment sql

44317f2

zqWu marked this pull request as draft March 20, 2023 14:36

gateway backend, insert timestamp '' => ''

b47655e

zqWu closed this Mar 21, 2023

zqWu mentioned this pull request Mar 21, 2023

mock sqlgateway #61

Closed

		return MockFlinkSqlGatewayClient.router.all_statements()


		def sql_equivalent(s1: str, s2: str) -> bool:

mock flink sql gateway client #59

mock flink sql gateway client #59

Conversation

zqWu commented Mar 15, 2023 • edited Loading

Description

PR Checklist

gliter commented Mar 15, 2023

zqWu commented Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

zqWu Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zqWu commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gliter commented Mar 16, 2023

zqWu commented Mar 17, 2023

zqWu commented Mar 17, 2023 • edited Loading

gliter commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gliter commented Mar 20, 2023

zqWu commented Mar 21, 2023

zqWu commented Mar 15, 2023 •

edited

Loading

zqWu commented Mar 15, 2023 •

edited

Loading

zqWu Mar 15, 2023 •

edited

Loading

zqWu commented Mar 17, 2023 •

edited

Loading