sql-machine-learning / sqlflow

Brings SQL and AI together.
https://sqlflow.org
Apache License 2.0
5.09k stars 699 forks source link

Shall we find a way to specify the external parser #2316

Closed weiguoz closed 4 years ago

weiguoz commented 4 years ago

Is your feature request related to a problem? Please describe. SQLFlow determines the external parser by dialect which specified in DataSource string. e.g. maxcompute Datasource string

For dialect maxcompute, we run the unit test well with calcite parser. However, in some company environments, it's more accurate to use the odps parser because of the Non-Open sourced syntax file.

https://github.com/sql-machine-learning/sqlflow/blob/436d4559577ea3f2214f9cf3b8ffd255f399d788/pkg/parser/external/parser.go#L32-L46

Describe the solution you'd like It looks like we should find a way to specify the external parser.

shendiaomo commented 4 years ago

How about this? It seems only maxcompute has the issue.

    case "mysql", "tidb":
        return newTiDBParser(), nil
    case "hive":
        return newJavaParser("hive"), nil
    case "calcite":
        return newJavaParser("calcite"), nil
    case "maxcompute":
        if parser := newJavaParser("odps"); parser != nil {
            return parser, nil
        }
        return newJavaParser("maxcompute"), nil
    case "alisa":
        return newJavaParser("odps"), nil
weiguoz commented 4 years ago

Hi @shendiaomo, thanks for your suggestion. To test if newJavaParser("odps") is nil, we need to try to call the Parser firstly. Let me do some tests.

sneaxiy commented 4 years ago

To test if newJavaParser("odps") is nil, we need to try to call the Parser firstly.

Maybe we can wrap a new Parser which is something like:

type ComposedParser struct {
    parsers []Parser
} 

func (parser *ComposedParser) Parse(stmt string) {
    for _, p := range parsers {
        ret, err := p.Parse(stmt)
        if err == nil {
            return ret
        }
    }
    return fmt.Errorf("...")
}
lhw362950217 commented 4 years ago

If we can't use odps parser, why should we add a maxcompute unit test run? Sorry, I'm not clear about the historical context.

weiguoz commented 4 years ago

To test if newJavaParser("odps") is nil, we need to try to call the Parser firstly.

Maybe we can wrap a new Parser which is something like:

type ComposedParser struct {
    parsers []Parser
} 

func (parser *ComposedParser) Parse(stmt string) {
    for _, p := range parsers {
        ret, err := p.Parse(stmt)
        if err == nil {
            return ret
        }
    }
    return fmt.Errorf("...")
}

This approach, each stmt calls multi parsers. I would like to use a fixed parser currently.