Skip to content

Conversation

@StpMax
Copy link
Collaborator

@StpMax StpMax commented Jul 10, 2025

Implementation of TABLE statement

TABLE table_name 
    [ORDER BY column_name] 
    [LIMIT number [OFFSET number]]

@entelligence-ai-pr-reviews
Copy link

Review Summary

🏷️ Draft Comments (3)

Skipped posting 3 drafted comments based on your review threshold. Feel free to update them here.

mindsdb_sql_parser/ast/table.py (1)

49-52: if self.limit: and if self.offset: will skip valid falsy values like 0, causing incorrect SQL for LIMIT 0 or OFFSET 0.

Scores:

  • Production Impact: 0
  • Fix Specificity: 0
  • Urgency Impact: 0
  • Total Score: 0

Reason for filtering: Line numbers are incorrect. The comment references lines 49-52 which contain code from the to_tree() method, but the actual issue exists in the to_string() method at lines 69-72. The bug description and fix are technically correct, but the line numbers don't match the described code location.

Analysis: While the technical issue is valid (falsy value handling for LIMIT/OFFSET), the comment must be removed due to incorrect line number mapping. The specified lines 49-52 are in the to_tree() method, not the to_string() method where the actual issue exists.


mindsdb_sql_parser/parser.py (2)

2089-2097: table_opt_offset is always required in the TABLE statement rule, so OFFSET cannot appear unless LIMIT is also present, which is not standard SQL behavior.

Scores:

  • Production Impact: 3
  • Fix Specificity: 4
  • Urgency Impact: 2
  • Total Score: 9

Reason for filtering: This is a valid technical issue. The TABLE statement rule at lines 2089-2097 requires all optional components (table_opt_order_by, table_opt_limit, table_opt_offset) to be present, which means OFFSET cannot be used without LIMIT. This is indeed non-standard SQL behavior. The suggested fix properly addresses this by adding multiple rule variants and using getattr for optional fields.

Analysis: The issue correctly identifies a parser limitation where TABLE statements must include all optional clauses. This could cause parsing failures for valid SQL. The fix is specific and well-structured, though the production impact is moderate since TABLE statements may not be heavily used.


60-109: The function query (lines 60-109) is over 40 lines and handles all top-level statements, making it difficult to maintain and extend as new statements are added.

Scores:

  • Production Impact: 1
  • Fix Specificity: 1
  • Urgency Impact: 1
  • Total Score: 3

Reason for filtering: While the comment correctly identifies that the query function is large (47 lines), the suggested fix is essentially a no-op comment that doesn't actually implement any refactoring. The commitable suggestion would not improve the code and just adds a comment without making any structural changes. This type of 'suggestion' wastes developer time.

Analysis: The observation about function size is valid, but the suggested fix is ineffective - it only adds a comment without implementing the recommended refactoring. This makes the suggestion unhelpful and potentially misleading to developers who might think applying it would solve the issue.


@github-actions
Copy link

github-actions bot commented Jul 10, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
mindsdb_sql_parser
   __about__.py10100%1–10
   __init__.py1192381%44, 48, 53, 98, 115, 118, 139–158, 165–166
   lexer.py2822193%370, 372, 374, 386, 388, 390, 396–414
   logger.py19479%14, 17, 23, 26
   parser.py11003097%129, 133, 288, 313, 419, 605, 622, 646–647, 868, 922, 999, 1100, 1153, 1163, 1202–1203, 1232, 1243, 1326, 1402, 1441, 1477, 1670–1671, 2013, 2021, 2074–2077
   utils.py46491%73–79
mindsdb_sql_parser/ast
   base.py36586%13, 28, 31, 46, 51
   create.py801285%23–31, 92–97
   drop.py52296%10, 13
   insert.py63494%39–41, 46
   show.py48198%18
   update.py53591%40–42, 75–76
mindsdb_sql_parser/ast/mindsdb
   knowledge_base.py97199%80
mindsdb_sql_parser/ast/select
   case.py38295%19, 22
   constant.py36197%23
   data.py11464%10–12, 15, 19
   identifier.py771186%47, 91–99, 109
   native_query.py13192%25
   operation.py145497%61, 70, 182, 206
   parameter.py15287%17, 20
   select.py109397%160–165
   star.py12283%8–9
TOTAL338915296% 

Tests Skipped Failures Errors Time
302 0 💤 0 ❌ 0 🔥 13.251s ⏱️

@StpMax StpMax requested a review from ea-rus July 10, 2025 13:16
from mindsdb_sql_parser.ast.base import ASTNode


class Table(ASTNode):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

table a is the same as select * from a, why to not parse it as CreateTable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you mean Select.
Indeed, select a and select * from a are equivalent. I prefer to have separate Table class because Select and Table are separate statements. However, it would be much easier to render this statement to different dialects if it will be Select, so i changed it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I meant Select

@StpMax StpMax requested a review from ea-rus July 14, 2025 13:26
@StpMax StpMax merged commit 570dbca into main Jul 18, 2025
14 checks passed
@StpMax StpMax deleted the feat-CONN-13 branch July 18, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants