Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add query settings interface #46

Merged
merged 9 commits into from
Sep 29, 2019
Merged

add query settings interface #46

merged 9 commits into from
Sep 29, 2019

Conversation

Yancey1989
Copy link
Collaborator

@Yancey1989 Yancey1989 commented Jul 27, 2019

fixed #45

@Yancey1989 Yancey1989 requested a review from weiguoz July 27, 2019 08:52
driver.go Outdated Show resolved Hide resolved
driver.go Outdated
type Driver struct{}
// ODPSDriver impls database/sql/driver.Driver
type ODPSDriver struct {
conn odpsConn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it is reasonable to maintain a conn inside the driver.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's okay, we need to maintain the settings struct in the DB object and the Driver struct can be exposed when we implement the sql.database.

driver.go Outdated
// SetQuerySettings sets the global query settings.
// TODO(Yancey1989): add the one-off query settings interface.
func (o MaxcomputeDriver) SetQuerySettings(hints map[string]string) error {
o.conn.hints = hints
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deep copy or shallow copy?

Copy link
Collaborator

@tonyyang-svail tonyyang-svail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yancey1989 @weiguoz Please be aware that after this change, odpsConn is no longer reentrant. Multiple goroutines can't share the same odpsConn. For example,

db := sql.Open("maxcompute", ...)

go func() {
    db.Driver().(*MaxcomputeDriver).SetQuerySettings(...)
}()

go func() {
    db.Query(...) // Oops, the setting has been changed. Or even worse, panic due to the race on hints.
}()

I like the idea // TODO(Yancey1989): add the one-off query settings interface.. I am wondering if the following implementation is possible.

package maxcompute

func QueryWithHint(db sql.DB, query string, hints map[string]string) (driver.Rows, error)  {
    conn, ok := db.Driver().(*MaxcomputeDriver)
    if !ok {
        return nil, fmt.Errorf(...)
    }
    newConn := odpsConn{conn.Client, conn.cfg, hints}
    return newConn.Query(query, ...)
}

Then in another package, for example

package main

func main() {
    db := Open("maxcompute", ...)
    query := ...
    hint := ...
    rows, err := maxcompute.QueryWithHint(conn, query, hints)
}

@Yancey1989
Copy link
Collaborator Author

Yancey1989 commented Jul 29, 2019

Thanks for @tonyyang-svail 's reminding, in the pyodps, there are two ways to set the query settings, the global one:

>>> from odps import options
>>> options.sql.settings = {'odps.sql.mapper.split.size': 16}
>>> o.execute_sql('select * from pyodps_iris')  # 会根据全局配置添加hints

and the one-off one:

o.execute_sql('select * from pyodps_iris', hints={'odps.sql.mapper.split.size': 16})

ref: https://pyodps.readthedocs.io/zh_CN/latest/base-sql.html#sql-hints

Maybe gomaxcompute need to implement both the two APIs. I think QueryWithHint is a good way to implement the one-off API, will implement it in this PR.

driver_test.go Outdated
a := assert.New(t)
db, err := sql.Open("maxcompute", cfg4test.FormatDSN())
a.NoError(err)
db.Driver().(*MaxcomputeDriver).SetQuerySettings(map[string]string{"odps.sql.mapper.split.size": "16"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test verify the hints is effective?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we also test odps.sql.mode = script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test verify the hints is effective?

No, it's not easy to test odps.sql.mapper.split.size on the client-side. But maybe test UDF script is good way, will add this one.

@Yancey1989
Copy link
Collaborator Author

Yancey1989 commented Jul 29, 2019

@tonyyang-svail
Copy link
Collaborator

@Yancey1989 btw, do we need to support logging on a job?

@Yancey1989
Copy link
Collaborator Author

Yancey1989 commented Jul 30, 2019

@Yancey1989 btw, do we need to support logging on a job?

@tonyyang-svail , I think we need that, the logs message is useful such as the job view link.

@weiguoz weiguoz self-requested a review September 3, 2019 07:56
weiguoz
weiguoz previously approved these changes Sep 3, 2019
Copy link
Collaborator

@weiguoz weiguoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Yancey1989
Copy link
Collaborator Author

Hi @tonyyang-svail , I file an issue to talk about the one-off API #50, maybe we can continue to talk about the API design there.

@Yancey1989
Copy link
Collaborator Author

Hi @weiguoz @tonyyang-svail @typhoonzero
I update this PR that define the query hints arguments in DSN:

access_id:[email protected]/api?curr_project=test_ci&scheme=http&hints_odps.sql.mapper.split_size=16

the URL query arguments key has prefix hints_ would be converted to query hints argument.

dsn.go Outdated
for k, v := range querys {
// The query args such as hints_odps.sql.mapper.split_size=16
// would be converted to the maxcompute query hints: {"odps.sql.mapper.split_size": "16"}
if strings.HasPrefix(k, "hints_") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use odps. as prefix?

Copy link
Collaborator Author

@Yancey1989 Yancey1989 Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can't, some case like UDF prediction job would set the query hints using mst as the prefix:

set mst.model.path=oss://<oss path>;
set mst.model.name=<model name>

dsn.go Outdated Show resolved Hide resolved
typhoonzero
typhoonzero previously approved these changes Sep 10, 2019
Copy link
Collaborator

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dsn.go Show resolved Hide resolved
@typhoonzero
Copy link
Collaborator

Is this ready to merge? @Yancey1989

@Yancey1989 Yancey1989 merged commit 402feb9 into develop Sep 29, 2019
@Yancey1989 Yancey1989 deleted the sql_settings branch September 29, 2019 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support settings for SQL
4 participants