Expanding YQL Syntax support in sqlc engine #10
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR significantly expands YQL (Yandex Query Language) syntax support in the sqlc engine by implementing multiple statement types and enhancing the SELECT statement parser. The changes focus on adding comprehensive support for DDL operations, DML operations, and improving the robustness of SELECT statement parsing.
- Implements support for 14 new YQL statement types including ALTER TABLE, DO, and various user/group management statements
- Enhances SELECT statement parsing with support for UNION operations, ORDER BY, LIMIT/OFFSET, GROUP BY with ROLLUP/CUBE, and HAVING clauses
- Refactors utility functions to improve naming consistency and adds new parsing functions for table identifiers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/engine/ydb/utils.go | Refactors identifier parsing functions and adds new parseAnIdTable function |
| internal/engine/ydb/convert.go | Implements converters for ALTER TABLE and DO statements, significantly enhances SELECT statement parsing with comprehensive clause support |
| internal/engine/ydb/catalog_tests/select_test.go | Updates test cases to match new SELECT statement AST structure with additional fields |
| internal/engine/ydb/catalog_tests/delete_test.go | Updates DELETE test case to include new SELECT statement fields |
Comments suppressed due to low confidence (1)
internal/engine/ydb/convert.go:1
- [nitpick] These error messages should be more descriptive about what the user should do. Consider messages like 'YDB: INTERSECT operation is not yet supported in this version' or provide guidance on alternatives.
package ydb
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Subtype: ast.AT_DropColumn, | ||
| }) | ||
| } | ||
| case action.Alter_table_alter_column_drop_not_null() != nil: |
There was a problem hiding this comment.
как будто бы можно объединить с action.Alter_table_drop_column() и избежать дублирования
There was a problem hiding this comment.
Кажется там будет больше геморроя их объединять 😄 . У них единственное общее та - an_id, который к тому же из разных структур достается, мне кажется бессмысленная оптимизация
| } | ||
|
|
||
| switch { | ||
| case n.Call_action() != nil: |
There was a problem hiding this comment.
можно каждой ветке в switch выделить по отдельному методу для упрощения кода
There was a problem hiding this comment.
Я на самом деле специально так сделал. В грамматике call_action и inline_action встречаются 1 раз - тут. И вот обычно, когда приходится обрабатывать такие одноразовые штуки, я прям так и записываю их, чтоб не путать человека, который в будущем код будет допиливать. Не знаю если честно, валиден такой подход или нет
2 participants
pragma_stmtselect_stmtnamed_nodes_stmtcreate_table_stmtdrop_table_stmtuse_stmtinto_table_stmtcommit_stmtupdate_stmtdelete_stmtrollback_stmtdeclare_stmtimport_stmtexport_stmtalter_table_stmt(upd)alter_external_table_stmtdo_stmt(upd)define_action_or_subquery_stmtif_stmtfor_stmtvalues_stmtcreate_user_stmtalter_user_stmtcreate_group_stmtalter_group_stmtdrop_role_stmtcreate_object_stmtalter_object_stmtdrop_object_stmtcreate_external_data_source_stmtalter_external_data_source_stmtdrop_external_data_source_stmtcreate_replication_stmtdrop_replication_stmtcreate_topic_stmtalter_topic_stmtdrop_topic_stmtgrant_permissions_stmtrevoke_permissions_stmtalter_table_store_stmtupsert_object_stmtcreate_view_stmtdrop_view_stmtalter_replication_stmtcreate_resource_pool_stmtalter_resource_pool_stmtdrop_resource_pool_stmtcreate_backup_collection_stmtalter_backup_collection_stmtdrop_backup_collection_stmtanalyze_stmtcreate_resource_pool_classifier_stmtalter_resource_pool_classifier_stmtdrop_resource_pool_classifier_stmtbackup_stmtrestore_stmtalter_sequence_stmtcreate_transfer_stmtalter_transfer_stmtdrop_transfer_stmtalter_database_stmtshow_create_table_stmt