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

copr: Add enum/set support for codegen #9133

Merged
merged 20 commits into from
Dec 9, 2020
Merged

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Nov 27, 2020

Signed-off-by: Xuanwo github@xuanwo.io

What problem does this PR solve?

Add enum support in codegen, after this PR merged, we can implement copr functions like:

#[rpn_fn]
fn cast_set_as_int(val: SetRef) -> Result<Option<Int>> {
    Ok(Some(val.value() as i64))
}

Check List

Tests

  • Unit test
  • Integration test

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 3, 2020

Let's merge this PR into previous one #9106

@Xuanwo Xuanwo closed this Dec 3, 2020
@Xuanwo Xuanwo reopened this Dec 3, 2020
@Xuanwo Xuanwo marked this pull request as ready for review December 3, 2020 06:44
@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 3, 2020

PTAL @skyzh @zhongzc @andylokandy

@Xuanwo Xuanwo changed the title copr: Add set support for codegen copr: Add enum/set support for codegen Dec 3, 2020
Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

LGTM. What about merging logic of xxxRef? (e.g. pack them into an enum), so as to reduce duplicated code?

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 3, 2020
@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 3, 2020

What about merging logic of xxxRef? (e.g. pack them into an enum), so as to reduce duplicated code?

Worth a try, maybe we can implement it in another PR.

@skyzh skyzh self-requested a review December 4, 2020 06:33
@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 4, 2020

ping @skyzh to take a look

Signed-off-by: Xuanwo <github@xuanwo.io>
Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Great!

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 7, 2020

ping @andylokandy , PTAL

@andylokandy
Copy link
Contributor

andylokandy commented Dec 8, 2020

Generally LGTM, please add a test for the macro expansion result, 'cause i think it can help new comer figure out what exactly the macro does.

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 8, 2020

Maybe I can add a macro expansion test for rpn_fn in another PR? (I think this test must be very verbose).

@andylokandy
Copy link
Contributor

andylokandy commented Dec 8, 2020

However, I think the tests are tightly related to the pr itself unlike additional improvement.

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 8, 2020

OK, I will add a test here.

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@andylokandy
Copy link
Contributor

If an ident in a big token tree goes wrong, it'll be hard to locate. I suggest renaming to token_tree_equal, to return the bool (sorry for the last comment) and assert it in the test.

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 8, 2020

If an ident in a big token tree goes wrong, it'll be hard to locate. I suggest renaming to token_tree_equal, to return the bool (sorry for the last comment) and assert it in the test.

I will look into this to find a solution for macro debuging.

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 9, 2020

Debug message looks like following:

expect: TokenStream [
    Ident {
        sym: row_index,
    },
    Punct {
        char: ',',
        spacing: Alone,
    },
    Ident {
        sym: vaxl,
    },
], actual: TokenStream [
    Ident {
        sym: row_index,
    },
    Punct {
        char: ',',
        spacing: Alone,
    },
    Ident {
        sym: val,
    },
]
thread 'rpn_function::tests_normal::test_enum_fn_generate_real_fn_trait_impl' panicked at 'expect: TokenStream [
    Ident {
        sym: row_index,
    },
    Punct {
        char: ',',
        spacing: Alone,
    },
    Ident {
        sym: vaxl,
    },
], actual: TokenStream [
    Ident {
        sym: row_index,
    },
    Punct {
        char: ',',
        spacing: Alone,
    },
    Ident {
        sym: val,
    },
]', components/tidb_query_codegen/src/rpn_function.rs:1747:9

Maybe this is enough? I found it's hard to get the token's location via public APIs.

Signed-off-by: Xuanwo <github@xuanwo.io>
Copy link
Contributor

@andylokandy andylokandy left a comment

Choose a reason for hiding this comment

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

LGTM, well done.

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 9, 2020
@andylokandy
Copy link
Contributor

/merge

@skyzh skyzh added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Xuanwo merge failed.

@andylokandy
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 1df1cad into tikv:master Dec 9, 2020
Little-Wallace added a commit to Little-Wallace/tikv that referenced this pull request Dec 10, 2020
commit cea0f29
Author: Connor <zbk602423539@gmail.com>
Date:   Thu Dec 10 06:37:22 2020 +0800

    tikv_util: Extract collections from tikv_util to avoid circular dependencies (tikv#9206)

    Signed-off-by: Connor <zbk602423539@gmail.com>

commit 1df1cad
Author: Xuanwo <github@xuanwo.io>
Date:   Wed Dec 9 21:25:52 2020 +0800

    copr: Add enum/set support for codegen (tikv#9133)

    Signed-off-by: Xuanwo <github@xuanwo.io>

commit e402023
Author: wangggong <793160615@qq.com>
Date:   Wed Dec 9 14:55:48 2020 +0800

    copr: (non-)vectorized compress & uncompress (tikv#9141)

    Signed-off-by: wangggong <793160615@qq.com>

commit b8fca7c
Author: Yilin Chen <sticnarf@gmail.com>
Date:   Tue Dec 8 16:06:17 2020 +0800

    txn: support fallback from async commit (take 2) (tikv#9196)

    Signed-off-by: Yilin Chen <sticnarf@gmail.com>

commit d7b47fa
Author: Jay <BusyJay@users.noreply.github.com>
Date:   Tue Dec 8 15:37:47 2020 +0800

    *: update dep (tikv#9201)

    Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

commit 5777bb6
Author: Nick Cameron <nrc@ncameron.org>
Date:   Tue Dec 8 15:39:18 2020 +1300

    txn: some async commit unit tests (tikv#9183)

    Signed-off-by: Nick Cameron <nrc@ncameron.org>

commit 0f135cb
Author: Liqi Geng <gengliqiii@gmail.com>
Date:   Mon Dec 7 09:33:49 2020 -0600

    raftstore: use async ready raft (tikv#9177)

    Signed-off-by: gengliqi <gengliqiii@gmail.com>

commit 0632bd2
Author: 5kbpers <tangminghua@pingcap.com>
Date:   Mon Dec 7 18:03:18 2020 +0800

    cdc: compatible with hibernate region (tikv#8907)

    Signed-off-by: 5kbpers <tangminghua@pingcap.com>

commit 623559a
Author: Long Deng <37360259+ldeng-ustc@users.noreply.github.com>
Date:   Mon Dec 7 17:07:18 2020 +0800

    copr: index scan return extra partiton id column when reading global index (tikv#8603)

    Signed-off-by: ldeng <ldeng@mail.ustc.edu.cn>

commit e146fae
Author: yiwu-arbug <yiwu@pingcap.com>
Date:   Sun Dec 6 21:36:18 2020 -0800

    encryption: Add config to gate the log style file dictionary optimization (tikv#9191)

    Signed-off-by: Yi Wu <yiwu@pingcap.com>

commit 4a41744
Author: Pramod Biligiri <pramodbiligiri@gmail.com>
Date:   Mon Dec 7 09:38:48 2020 +0530

    txn: code level docs for prewrite.rs (tikv#9189)

    Signed-off-by: Pramod Biligiri <pramodbiligiri@gmail.com>

commit 3bae18b
Author: Xuanwo <github@xuanwo.io>
Date:   Sat Dec 5 00:21:49 2020 +0800

    copr: Implement ENUM/SET support for aggr AVG  (tikv#9186)

    Signed-off-by: Xuanwo <github@xuanwo.io>

commit 21df48d
Author: Xinye Tao <xy.tao@outlook.com>
Date:   Fri Dec 4 17:34:46 2020 +0800

    tests: stabilize lease related tests (tikv#8955)

    Signed-off-by: tabokie <xy.tao@outlook.com>

commit 3326368
Author: Xuanwo <github@xuanwo.io>
Date:   Fri Dec 4 17:13:48 2020 +0800

    copr: Implement SET support for MAX/MIN/SUM (tikv#9184)

    Signed-off-by: Xuanwo <github@xuanwo.io>

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@Xuanwo Xuanwo deleted the copr-set-codegen branch February 7, 2021 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants