search

GreptimeTeam / greptimedb

An open-source, cloud-native, unified time series database for metrics, logs and events with SQL/PromQL supported. Available on GreptimeCloud.
https://greptime.com/
Apache License 2.0
4.18k stars 298 forks source link

Can't create view with range query #4358

Open killme2008 opened 1 month ago

killme2008 commented 1 month ago

What type of bug is this?

Unexpected error

What subsystems are affected?

Query Engine

Minimal reproduce step

For example:

CREATE VIEW test as SELECT
SELECT 
  ts, 
  host, 
  approx_percentile_cont(latency, 0.95) RANGE '5s' AS p95_latency 
FROM 
  app_metrics
ALIGN '5s' FILL PREV;

It fails with:

1: Table operation error, at src/frontend/src/instance.rs:232:14
2: Failed to convert between logical plan and substrait plan, at /Users/dennis/programming/rust/greptimedb/src/operator/src/statement/ddl.rs:451:14
3: Failed to encode DataFusion plan, at /Users/dennis/programming/rust/greptimedb/src/common/substrait/src/df_substrait.rs:80:43
4: NotImplemented("Serizlize logical plan for RangeSelect")

The RangeSelect plan can't be serizlized.

What did you expect to see?

Internal error

What did you see instead?

Create success

What operating system did you use?

Irrelevant.

What version of GreptimeDB did you use?

main branch

Relevant log output and stack trace

No response

etolbakov commented 1 month ago

I did an initial assessment and would like to share some findings: 1) the relevant code fragment that should be extended is src/common/substrait/src/extension_serializer.rs

          name if name == RangeSelect::name() => {
                let range_select = node
                    .as_any()
                    .downcast_ref::<RangeSelect>()
                    .expect("Failed to downcast to RangeSelect");
                Ok(range_select.serialize())

2) the RangeSelect struct is already defined in src/query/src/range_select/plan.rs. I came across the circular dependency when I was trying to use it, so probably moving this code under common::query would help.

3) to implement the serialize method one needs to create the proto definition in greptime-proto repo.

cc @waynexia

killme2008 commented 1 month ago

Yep.👍🏻

etolbakov commented 1 month ago

If the plan seems ok I'll try looking at proto definition in the background.

killme2008 commented 1 month ago

If the plan seems ok I'll try looking at proto definition in the background.

That would be cool!

killme2008 commented 1 month ago

@etolbakov Hi, are you still working on this issue? I think it's a fatal issue that needs to be addressed.

etolbakov commented 1 month ago

sorry Dennis, didn't make much progress to be honest. I will have time these weekend, but I appreciate the urgency, don't want to block you.

killme2008 commented 1 month ago

sorry Dennis, didn't make much progress to be honest. I will have time these weekend, but I appreciate the urgency, don't want to block you.

Don't worry. I think @waynexia could help you figure out how to address this issue. Please take a look.

Subsrait with DataFusion has so many issues, that we need to push the community to address them.