Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Implemented unit tests for various SQL operations including joins, aggregates, and select statements in exec_query_test.go. - Created a memory-based key-value store for testing purposes in test_helpers_test.go. - Added helper functions to facilitate executor setup and execution of plans. - Ensured comprehensive coverage for aggregate functions and handling of NULL values in queries.
Souvik606
left a comment
Souvik606
left a comment
There was a problem hiding this comment.
Check the review comments and fix them.
| bv, ok := b.(string) | ||
| if !ok { | ||
| return 0, fmt.Errorf("%w: cannot compare string with %T", ErrTypeMismatch, b) | ||
| } | ||
| return cmpOrdered(av, bv), nil |
There was a problem hiding this comment.
ast.TypeDecimal values are stored and evaluated as Go string types (e.g. "10.0" and "2.5"). When compareValues compares two decimal strings, it uses standard lexicographical string comparison (cmpOrdered(av, bv)).
Lexicographical comparison evaluates "10.0" < "2.5" as true (because ASCII character '1' comes before '2'). Queries like WHERE decimal_col > 2.5 fail to return correct rows.
Suggested Solution:Implement explicit decimal/numeric string parsing
case string:
bv, ok := b.(string)
if !ok {
return 0, fmt.Errorf("%w: cannot compare string with %T", ErrTypeMismatch, b)
}
fa, errA := strconv.ParseFloat(av, 64)
fb, errB := strconv.ParseFloat(bv, 64)
if errA == nil && errB == nil {
return cmpOrdered(fa, fb), nil
}
return cmpOrdered(av, bv), nil
| case int32: | ||
| bv, err := toInt64(b) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return cmpOrdered(int64(av), bv), nil | ||
| case int64: | ||
| bv, err := toInt64(b) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return cmpOrdered(av, bv), nil |
There was a problem hiding this comment.
When comparing an int32/int64 value a against a float32/float64 value b, compareValues converts b using toInt64(b). toInt64 truncates the floating-point number (e.g., 2.7 becomes 2).
Evaluating int32(2) == float64(2.7) calls toInt64(2.7)
Suggested Solution: Promote integer operands to float64 when comparing against floating-point types
| var nextSeq uint64 | ||
| if schema.HasSnowflakeID { | ||
| nextSeq, err = e.readSequence(ctx, plan.Database, plan.Table) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Snowflake ID generation reads the current sequence counter from storage (e.readSequence), increments it in local memory per row (*nextSeq++), and writes the updated counter back to storage at the end of the batch.
If two concurrent INSERT queries execute simultaneously, both read the same sequence value N. Both generate identical ID ranges N+1, N+2... and overwrite each other's IDs in storage.
Suggested Solution: Use atomic sequence allocation (e.g., atomic fetch-and-add via KV CAS operation or dedicated sequence mutex) to reserve ID blocks safely under concurrency.
| } | ||
|
|
||
| // execDropTable executes a DROP TABLE plan. | ||
| func (e *Executor) execDropTable(ctx context.Context, plan *planner.DropTablePlan) (*Result, error) { |
There was a problem hiding this comment.
execDropTable deletes table rows (appendTableDeleteOps) and table metadata (BuildDropTableOps), but fails to delete the sequence counter key (EncodeCatalogSeqKey(db, table)).Dropping a table leaves an orphaned sequence key in the KV store forever. Re-creating a table with the same name reuses the stale sequence counter instead of resetting to 0.
Suggested Solution: In execDropTable, encode the catalog sequence key and append a kv.OpDelete for seqKey to the batch.
| _, err = e.kv.Get(ctx, rowKey) | ||
| if err == nil { | ||
| return kv.Op{}, fmt.Errorf("%w: key already exists in %q.%q", ErrDuplicateKey, db, table) | ||
| } | ||
| if !errors.Is(err, kv.ErrKeyNotFound) { | ||
| return kv.Op{}, fmt.Errorf("executor: checking PK existence: %w", err) | ||
| } |
There was a problem hiding this comment.
prepareInsertOp checks if a primary key exists via e.kv.Get(ctx, rowKey). If ErrKeyNotFound, it appends a kv.OpPut to the batch.
Two parallel INSERT requests inserting the same primary key will both see ErrKeyNotFound during their Get phase. Both will append OpPut and proceed to execute WriteBatch, silently overwriting existing data without raising ErrDuplicateKey.
## Issue Reference Fixes #42 <!-- This will be automatically replaced by a summary generated by CodeRabbitAI --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for interacting with key-value data through a remote storage service. - Supports reading, writing, deleting, and batch-updating entries. - Added prefix-based scanning with ordered iteration and safe stream cleanup. - Missing keys now return a consistent not-found result. - **Tests** - Added coverage for standard operations, batch updates, scans, empty results, and missing-key handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Issue Reference
@coderabbitai summary