-
Notifications
You must be signed in to change notification settings - Fork 6.2k
importsdk, importer: fix sampled source size in import estimate #67492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
feed1c7
ce12082
a6bf749
b62a461
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -393,13 +393,13 @@ func (s *fileScanner) buildEstimateTableInfo(ctx context.Context, tblMeta *mydum | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| p := parser.New() | ||||||||||||||||||||||||||||||||||||
| p.SetSQLMode(s.config.sqlMode) | ||||||||||||||||||||||||||||||||||||
| stmt, err := p.ParseOneStmt(schemaSQL, "", "") | ||||||||||||||||||||||||||||||||||||
| stmts, _, err := p.ParseSQL(schemaSQL) | ||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||
| return nil, errors.Trace(err) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| createStmt, ok := stmt.(*ast.CreateTableStmt) | ||||||||||||||||||||||||||||||||||||
| if !ok { | ||||||||||||||||||||||||||||||||||||
| return nil, errors.Errorf("schema file %s does not contain a CREATE TABLE statement", tblMeta.SchemaFile.FileMeta.Path) | ||||||||||||||||||||||||||||||||||||
| createStmt, err := buildEstimateCreateTableStmt(stmts, tblMeta) | ||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||
| return nil, err | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| tableInfo, err := ddl.BuildTableInfoFromAST(metabuild.NewContext(), createStmt) | ||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -408,6 +408,49 @@ func (s *fileScanner) buildEstimateTableInfo(ctx context.Context, tblMeta *mydum | |||||||||||||||||||||||||||||||||||
| return tableInfo, nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func buildEstimateCreateTableStmt(stmts []ast.StmtNode, tblMeta *mydump.MDTableMeta) (*ast.CreateTableStmt, error) { | ||||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||||
| firstCreateStmt *ast.CreateTableStmt | ||||||||||||||||||||||||||||||||||||
| createStmtCount int | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| for _, stmt := range stmts { | ||||||||||||||||||||||||||||||||||||
| createStmt, ok := stmt.(*ast.CreateTableStmt) | ||||||||||||||||||||||||||||||||||||
| if !ok { | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if firstCreateStmt == nil { | ||||||||||||||||||||||||||||||||||||
| firstCreateStmt = createStmt | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| createStmtCount++ | ||||||||||||||||||||||||||||||||||||
| if estimateCreateTableStmtMatchesMeta(createStmt, tblMeta) { | ||||||||||||||||||||||||||||||||||||
| return createStmt, nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if createStmtCount == 1 { | ||||||||||||||||||||||||||||||||||||
| return firstCreateStmt, nil | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if createStmtCount == 0 { | ||||||||||||||||||||||||||||||||||||
| return nil, errors.Errorf("schema file %s does not contain a CREATE TABLE statement", tblMeta.SchemaFile.FileMeta.Path) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return nil, errors.Errorf( | ||||||||||||||||||||||||||||||||||||
| "schema file %s contains %d CREATE TABLE statements but none match table %s.%s", | ||||||||||||||||||||||||||||||||||||
| tblMeta.SchemaFile.FileMeta.Path, | ||||||||||||||||||||||||||||||||||||
| createStmtCount, | ||||||||||||||||||||||||||||||||||||
| tblMeta.DB, | ||||||||||||||||||||||||||||||||||||
| tblMeta.Name, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func estimateCreateTableStmtMatchesMeta(createStmt *ast.CreateTableStmt, tblMeta *mydump.MDTableMeta) bool { | ||||||||||||||||||||||||||||||||||||
| if !strings.EqualFold(createStmt.Table.Name.String(), tblMeta.Name) { | ||||||||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if createStmt.Table.Schema.String() == "" { | ||||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return strings.EqualFold(createStmt.Table.Schema.String(), tblMeta.DB) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+444
to
+451
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle empty If Suggested fix func estimateCreateTableStmtMatchesMeta(createStmt *ast.CreateTableStmt, tblMeta *mydump.MDTableMeta) bool {
if !strings.EqualFold(createStmt.Table.Name.String(), tblMeta.Name) {
return false
}
- if createStmt.Table.Schema.String() == "" {
+ if createStmt.Table.Schema.String() == "" || tblMeta.DB == "" {
return true
}
return strings.EqualFold(createStmt.Table.Schema.String(), tblMeta.DB)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func sourceTypeToImportFormat(tp mydump.SourceType) (string, error) { | ||||||||||||||||||||||||||||||||||||
| switch tp { | ||||||||||||||||||||||||||||||||||||
| case mydump.SourceTypeCSV: | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 [Minor] Parquet and fallback source-size branches lack targeted regression coverage
Why
The patch introduces format-specific source-size logic in
sampledRowSourceSize, but the new test only validates the SQL consumed-bytes path and does not exercise the newly added Parquet or non-positive delta fallback branches.Scope
pkg/executor/importer/sampler.go:388; pkg/executor/importer/sampler_test.go:238
Risk if unchanged
Future parser position behavior changes can silently skew sampled source-size estimation for Parquet or edge parser offsets, which may mis-tune IMPORT resource planning without an obvious failure signal.
Evidence
sampledRowSourceSizeaddsif s.cfg.Format == DataFormatParquet { return int64(row.Length) }andif rowDelta := endPos - startPos; rowDelta > 0 { ... }fallback logic, while the added testsql_source_size_uses_consumed_bytes_not_buffered_progresscovers only SQL input.Change request
add UT for it: add a case for
DataFormatParquetand a case for the non-parquetendPos <= startPosfallback path so each new sizing branch is pinned by deterministic assertions.