Comparison aggregation unnest and where clause fix #5090

egor-ryashin · 2024-06-17T18:16:42Z

Also, added Druid CI testing

begelundmuller · 2024-06-20T11:48:46Z

runtime/reconcilers/metrics_view.go

+		if os.Getenv("DEBUG") == "1" {
+			fmt.Println("Reconciler error", validateErr.Error())
+		}


It took me several hours of debugging without those log messages, I don't understand. Reconciler and validation errors aren't shown if tests fail

You just need to uncomment this line to get reconciler logs from tests:

rill/runtime/testruntime/testruntime.go

Line 68 in 1983fb1

// logger, err := zap.NewDevelopment()

Unless it's a very special circumstance, we should only read ENV variables at the entrypoint (e.g. CLI command or test function) and then pass config explicitly to internal packages, and we should use the Zap logger consistently instead of fmt.Println.

Unless it's a very special circumstance, we should only read ENV variables at the entrypoint (e.g. CLI command or test function) I doubt that, in terms of speed of development I would rather add everywhere

if os.Getenv("DEBUG") == "1") { fmt.Println(sql) }

and forgot about adding/removing and checking fmt.Println(sql) all the time.
There's always one more place where one forgot to propagate .Debug variable and you need some hours of work to reverse-engineer and propagate it for a new test.

This is equivalent to using the debug logger. Whether you need to set DEBUG=1 or whether you uncomment the debug logger, it's the same overhead.

You can keep DEBUG=1 in a config (ie vscode config).

begelundmuller · 2024-06-20T11:48:49Z

runtime/reconcilers/project_parser.go

+			debug := os.Getenv("DEBUG")
+			if debug == "1" {
+				fmt.Println("Parser error", e.Message)
+			}


begelundmuller · 2024-06-20T11:50:38Z

runtime/testruntime/testruntime.go

@@ -244,7 +260,7 @@ func NewInstanceForDruidProject(t TestingT) (*runtime.Runtime, string) {
 			{
 				Type:   "druid",
 				Name:   "druid",
-				Config: map[string]string{"dsn": "http://localhost:8888/druid/v2/sql"},
+				Config: map[string]string{"dsn": fmt.Sprintf("https://%s:%[email protected]/druid/v2/sql", splits[0], splits[1])},


Remove – we should use test containers instead of running against a prod cluster in testing. If we really need to run against an external cluster, it should be a dedicated cluster and generically configured

I think tests are better than no tests

Yes we need to invest more in good testing for Druid, but we need to do it with proper abstractions so the tests are safe and easy to run for everybody. This both has the potential to overload a production cluster we rely on for more important use cases, makes it more difficult for folks internally to configure the tests, and it looks bad to open source contributors who don't get a chance to run or modify these tests.

and it looks bad to open source contributors who don't get a chance to run or modify these tests I will look even worse if we get production Druid-based errors
This both has the potential to overload a production cluster we rely on for more important use cases, makes it more difficult for folks internally to configure the tests @nishantmonu51 I wonder if we should start a dedicated cluster for CI tests?

A dedicated cluster would be better. Why do you consider a test container unsuitable for Druid testing? Regarding this PR, I think bigger changes to Druid testing improvements should be done in a dedicated PR.

There's a dedicated container implemented but it was switched off for performance reason. IE

=== RUN TestDruid druid_test.go:83: druid: skipping test in short mode --- SKIP: TestDruid (0.00s)

"Regarding this PR, I think bigger changes to Druid testing improvements should be done in a dedicated PR."
I expected it will be an easy addition but it turns we want to postpone Druid tests again - because we cannot find a perfect solution in short amount of time - we postpone any other solution entirely.

Does changing metrics-in to master-in cluster resolve the issue?

begelundmuller · 2024-06-20T11:51:54Z

runtime/queries/metricsview.go

@@ -76,7 +76,7 @@ func lookupMetricsView(ctx context.Context, rt *runtime.Runtime, instanceID, nam

 	res, err := ctrl.Get(ctx, &runtimev1.ResourceName{Kind: runtime.ResourceKindMetricsView, Name: name}, false)
 	if err != nil {
-		return nil, nil, status.Error(codes.InvalidArgument, err.Error())
+		return nil, nil, status.Error(codes.InvalidArgument, fmt.Sprintf("%s, %s", err.Error(), name))


Suggested change

return nil, nil, status.Error(codes.InvalidArgument, fmt.Sprintf("%s, %s", err.Error(), name))

return nil, nil, status.Error(codes.InvalidArgument, fmt.Sprintf("error getting metrics view %q: %s", name, err.Error()))

begelundmuller · 2024-06-20T11:53:29Z

runtime/metricsview/executor.go

@@ -189,6 +189,7 @@ func (e *Executor) Query(ctx context.Context, qry *Query, executionTime *time.Ti
 		if err != nil {
 			return nil, false, err
 		}
+		fmt.Println(sql)


begelundmuller · 2024-06-20T11:55:55Z

runtime/metricsview/astsql.go

+func (b *sqlBuilder) writeSelect(n *SelectNode, level int) error {
+	b.out.WriteString(strings.Repeat("\t", level))


Can we remove the pretty printing here? It's redundant extra length and allocations. If it's for debugging, there are many SQL formatters that can prettify a query (I use the adpyke.vscode-sql-formatter extension in VS Code)

Meaning additional manual work in exchange for nano-optimisation

I'm more worried about the need for understanding, maintaining and debugging formatting code for no reason. There are plenty of decent SQL formatter libraries for Go if we have cases where we need to print approximately formatted SQL.

egor-ryashin · 2024-07-03T11:11:46Z

CI tests part is moved here #5184

egor-ryashin · 2024-07-16T19:21:57Z

It's untrivial at this point to merge it without CI tests, so i'm closing. I've checked the critical errors (order by measure, unnest) were fixed in main branch already. Other bugs I'll work on in another ticket.

Egor Ryashin and others added 6 commits June 17, 2024 13:24

tests

6c0c106

unnest testing

bee9191

Merge branch 'main' into ca-unnest

cfadeef

Update go-test.yml

b8e19a3

unnest testing

e1fea87

Update go-test.yml

1e60e7b

egor-ryashin marked this pull request as ready for review June 18, 2024 17:38

egor-ryashin requested a review from begelundmuller June 18, 2024 17:38

Egor Ryashin and others added 3 commits June 18, 2024 20:55

unnest testing

e690fb6

Update go-test.yml

857a557

Update go-test.yml

ba7dbfe

egor-ryashin marked this pull request as draft June 18, 2024 18:02

egor-ryashin and others added 10 commits June 18, 2024 22:29

Update go-test.yml

45736a7

unnest testing - metrics-in

0f9b4cf

unnest testing - metrics-in

e0de929

Update go-test.yml

3da4da1

Update go-test.yml

2d902d6

Update go-test.yml

a0894cc

unnest testing - metrics-in

2a8a6d3

Update go-test.yml

1ee70ac

Merge branch 'main' into ca-unnest

427c348

unnest testing - metrics-in

4cbe4ec

egor-ryashin marked this pull request as ready for review June 19, 2024 20:11

egor-ryashin changed the title ~~Comparison aggregation unnest testing~~ Comparison aggregation unnest and where clause fix Jun 20, 2024

begelundmuller requested changes Jun 20, 2024

View reviewed changes

Egor Ryashin added 5 commits June 20, 2024 17:08

unnest testing - metrics-in

1c64a96

unnest testing - metrics-in

38d2edc

unnest testing - metrics-in

06a14df

unnest testing - metrics-in

1273389

unnest testing - metrics-in

c0bc796

unnest testing

398d782

egor-ryashin closed this Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comparison aggregation unnest and where clause fix #5090

Comparison aggregation unnest and where clause fix #5090

egor-ryashin commented Jun 17, 2024 •

edited

Loading

begelundmuller Jun 20, 2024

egor-ryashin Jun 20, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

egor-ryashin Jun 20, 2024 •

edited

Loading

begelundmuller Jun 21, 2024

egor-ryashin Jun 25, 2024 •

edited

Loading

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

egor-ryashin Jun 20, 2024

begelundmuller Jun 20, 2024 •

edited

Loading

egor-ryashin Jun 20, 2024 •

edited

Loading

begelundmuller Jun 21, 2024

egor-ryashin Jun 25, 2024 •

edited

Loading

egor-ryashin Jun 27, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

egor-ryashin Jun 20, 2024

begelundmuller Jun 20, 2024

egor-ryashin commented Jul 3, 2024

egor-ryashin commented Jul 16, 2024

	return nil, nil, status.Error(codes.InvalidArgument, fmt.Sprintf("%s, %s", err.Error(), name))
	return nil, nil, status.Error(codes.InvalidArgument, fmt.Sprintf("error getting metrics view %q: %s", name, err.Error()))

		func (b sqlBuilder) writeSelect(n SelectNode, level int) error {
		b.out.WriteString(strings.Repeat("\t", level))

Comparison aggregation unnest and where clause fix #5090

Comparison aggregation unnest and where clause fix #5090

Conversation

egor-ryashin commented Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begelundmuller Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

egor-ryashin Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egor-ryashin commented Jul 3, 2024

egor-ryashin commented Jul 16, 2024

egor-ryashin commented Jun 17, 2024 •

edited

Loading

egor-ryashin Jun 20, 2024 •

edited

Loading

egor-ryashin Jun 25, 2024 •

edited

Loading

begelundmuller Jun 20, 2024 •

edited

Loading

egor-ryashin Jun 20, 2024 •

edited

Loading

egor-ryashin Jun 25, 2024 •

edited

Loading