Documentation:Implement all intergration test#33
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive integration tests for various SQL operators including filter, join, aggregation, sort, and scalar functions. The changes convert previously commented-out test specifications into working test implementations, and add supporting functionality for boolean type handling in aggregations and scalar function validation in filters.
Key Changes:
- Implements 7 test functions covering filter/limit, scalar functions, sorting, joins, aggregations, distinct operations, and complex multi-operator pipelines
- Adds boolean type support in the aggregation builder
- Extends filter validation to handle scalar functions
- Removes unnecessary string conversions in expression tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/Backend/opti-sql-go/operators/test/intergration_test.go |
Main integration test implementation with 7 test functions covering various operator combinations |
src/Backend/opti-sql-go/operators/filter/filter.go |
Adds scalar function validation support and debug logging |
src/Backend/opti-sql-go/operators/aggr/groupBy.go |
Implements boolean type handling in dynamic array builder with helper function |
src/Backend/opti-sql-go/Expr/expr_test.go |
Removes redundant string conversions in test assertions |
src/Backend/opti-sql-go/Expr/expr.go |
Removes debug print statement from literal resolve function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if v == "true" || v == true { | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
The castToBool function only checks for "true" string and true boolean. This will incorrectly return false for the string "false", which should be considered a valid boolean value. Consider handling "false" explicitly and potentially raising an error for invalid inputs.
| return false | |
| if v == "false" || v == false { | |
| return false | |
| } | |
| panic(fmt.Sprintf("invalid value for castToBool: %v (type %T)", v, v)) |
| return | ||
| } |
There was a problem hiding this comment.
The t.Fatalf should use t.Errorf or the test should not have a return statement after it. t.Fatalf immediately terminates the test, so the return on line 287 is unreachable code.
| return | |
| } | |
| } | |
| } |
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if batch != nil { | ||
| t.Fatalf("was expecting an empty batch but recieved %s\n", batch.PrettyPrint()) |
There was a problem hiding this comment.
Spelling error: "recieved" should be "received".
| t.Fatalf("was expecting an empty batch but recieved %s\n", batch.PrettyPrint()) | |
| t.Fatalf("was expecting an empty batch but received %s\n", batch.PrettyPrint()) |
| case arrow.BOOL: | ||
| b := array.NewBooleanBuilder(mem) | ||
| for _, v := range values { | ||
| fmt.Printf("values:%v type:%T\n", v, v) |
There was a problem hiding this comment.
Debug print statement left in production code. This should be removed before merging as it will clutter logs during normal operation.
| fmt.Printf("values:%v type:%T\n", v, v) |
| if err != nil { | ||
| t.Fatalf("join init failed: %v", err) | ||
| } | ||
| fmt.Printf("schema:%v\n", j.Schema()) |
There was a problem hiding this comment.
Debug print statement left in production code. This should be removed before merging as it will clutter logs during normal operation.
| fmt.Printf("schema:%v\n", j.Schema()) | |
| t.Logf("schema:%v", j.Schema()) |
|
|
||
| case *Expr.NullCheckExpr: | ||
| return validPredicates(p.Expr, schema) | ||
| case *Expr.ScalarFunction: |
There was a problem hiding this comment.
The ScalarFunction case always returns true without validating its arguments. This could allow invalid scalar functions to pass validation. Consider recursively validating the function's arguments against the schema, similar to how BinaryExpr validates its children.
| case *Expr.ScalarFunction: | |
| case *Expr.ScalarFunction: | |
| for _, arg := range p.Args { | |
| if !validPredicates(arg, schema) { | |
| return false | |
| } | |
| } |
| exprs := Expr.NewExpressions( | ||
| Expr.NewAlias(Expr.NewColumnResolve("left_id"), "id"), | ||
| Expr.NewColumnResolve("username"), | ||
| Expr.NewAlias(Expr.NewColumnResolve("department_name"), "deptartment"), |
There was a problem hiding this comment.
Spelling error in alias: "deptartment" should be "department".
| Expr.NewAlias(Expr.NewColumnResolve("department_name"), "deptartment"), | |
| Expr.NewAlias(Expr.NewColumnResolve("department_name"), "department"), |
| if !arrow.TypeEqual(dt1, dt2) { | ||
| return false | ||
| } | ||
| fmt.Printf("left:\t%v\nright:\t%v\n", p.Left, p.Right) |
There was a problem hiding this comment.
Debug print statement left in production code. This should be removed before merging as it will clutter logs during normal operation.
| fmt.Printf("left:\t%v\nright:\t%v\n", p.Left, p.Right) |
| // (5.B) SELECT is_active, COUNT(*) AS active_count, AVG(age_years) AS avg_age FROM source1 GROUP BY is_active; | ||
| t.Run("5B", func(t *testing.T) { | ||
| src := source1Project() | ||
| fmt.Printf("\t%v\n", src.Schema()) |
There was a problem hiding this comment.
Debug print statement left in production code. This should be removed before merging as it will clutter logs during normal operation.
| fmt.Printf("\t%v\n", src.Schema()) | |
| t.Logf("\t%v\n", src.Schema()) |
|
closes #32 |
No description provided.