Adapt suite tests for cs #13204
Adapt suite tests for cs #13204
Conversation
zverevgeny
force-pushed
the
adapt_suite_tests_for_cs
branch
from
190b207 to
bdcdd21
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
| type = StatementDefinition._parse_statement_type(lines[0]) | ||
| if type is None: | ||
| type, table_types = StatementDefinition._parse_statement_type(lines[0]) | ||
| if type is None and table_types: |
There was a problem hiding this comment.
Логика неправильная мне кажется: пусть _parse_statement_type кидает когда не знает что с этим делать, а не вызывающая сторона. Когда условие было простым, было еще более менее, а сейчас стало хуже
+ я не совсем понимаю, почему мы должны игнорить непонятный тип, если у нас table_types пустой
There was a problem hiding this comment.
есть строчки вида:
statement skipped <произвольный текст>
Поэтому для skipped тестов не важен тип
There was a problem hiding this comment.
Выкидывать исключение можно и из _parse_statement_type, но тогда туда нужно имя сьюта и номер строки передавать только для исключения. Не уверен, что это сильно лучше
|
|
||
| return self.legacy_pool.retry_operation_sync(lambda s: s.explain(yql_text)).query_plan | ||
|
|
||
| def execute_query(self, statement: StatementDefinition, amended_text: str = None): |
There was a problem hiding this comment.
Я бы предложил написать коммент к функции, мол она выполняет запрос, и что scan/row/column/... результаты одинаковы
| return c | ||
| return 0 | ||
|
|
||
| sorted_result_row = sorted(flatten_result(result_row), key=cmp_to_key(compare)) |
There was a problem hiding this comment.
Я мб что-то не понимаю, но почему scan vs row мы не сортируем, а вот row vs column мы это делаем?
Возможно было бы правильнее сортировать безусловно для все случаев
There was a problem hiding this comment.
Это же ослабит проверку. Сейчас ожидается, что они совпадают и без дополнительной сортировки.
There was a problem hiding this comment.
А в чем смысл такой проверки? Если какой-нибудь ORDER BY не указан, то они точно должны совпадать?
There was a problem hiding this comment.
Не знаю. Я в DS не сильно ориентируюсь. Ну и странно ослаблять проверку DS при добавлении CS
There was a problem hiding this comment.
Так проверка некорректная, имхо (нужна сортировка)
Ты сделал корректно для cs (вероятно потому что иначе не проходило), но я не вижу причин не сделать точно так же для ds
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
2 participants
Changelog category