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

Implement groupby #749

Open
wants to merge 4 commits into
base: master
from

Conversation

@laura-ding
Copy link
Contributor

commented Aug 9, 2019

Closes #676

@laura-ding laura-ding force-pushed the laura-ding:groupby branch from cbedc44 to 6393eb4 Aug 10, 2019

@jude-zhu jude-zhu requested review from CPWstatic, dutor and whitewum Aug 12, 2019

@laura-ding laura-ding referenced this pull request Aug 13, 2019
@whitewum
Copy link
Contributor

left a comment

Great Job

std::string query = "GO FROM 1 OVER work "
"YIELD $$.company.name, $^.person.name "
"| GROUP BY $$.company.name "
"YIELD $$.company.name as name, "

This comment has been minimized.

Copy link
@whitewum

whitewum Aug 13, 2019

Contributor

use replace all as to AS
let's follow nGQL style

src/parser/test/ParserTest.cpp Show resolved Hide resolved
auto result = parser.parse(query);
ASSERT_TRUE(result.ok()) << result.status();
}
// All fun error, empty group name

This comment has been minimized.

Copy link
@whitewum

whitewum Aug 13, 2019

Contributor

I think currently we don't support in the following order,

YIELD xxx, sum(YYY) GROUP BY xxx

So let parser test make sure of it (in case any further questions).


TEST_F(GroupByLimitTest, GroupByOrderByLimitTest) {
}
} // namespace graph

This comment has been minimized.

Copy link
@whitewum

whitewum Aug 13, 2019

Contributor

so you will add some case including
group by, order by, and limit
would you?

This comment has been minimized.

Copy link
@laura-ding

laura-ding Aug 19, 2019

Author Contributor

yeah

"YIELD $-.id as id, "
"SUM(age) as age |"
"GO FROM $-.id OVER serve "
"YIELD $$.team.name as name";

This comment has been minimized.

Copy link
@whitewum

whitewum Aug 13, 2019

Contributor

YIELD $$.team.name AS name, $-.age AS sumAge to test propogation $- is correct.

double acc = 0.0;
double avg = avg_.getResult().get_double_precision();
std::for_each(std::begin(elems_), std::end(elems_), [&](const double elem) {
acc += (elem - avg) *(elem - avg);

This comment has been minimized.

Copy link
@whitewum

whitewum Aug 13, 2019

Contributor

good math
but space: * (

@@ -458,7 +458,7 @@ std::string PrimaryExpression::toString() const {
snprintf(buf, sizeof(buf), "%s", boost::get<bool>(operand_) ? "true" : "false");
break;
case VAR_STR:
return "\"" + boost::get<std::string>(operand_) + "\"";
return boost::get<std::string>(operand_);

This comment has been minimized.

Copy link
@monadbobo

monadbobo Aug 14, 2019

Contributor

need default.

// key : group col vals, val: cal funptr
using FunCols = std::vector<std::shared_ptr<AggFun>>;
using GroupData = std::unordered_map<ColVals, FunCols, ColsHasher>;
std::unordered_map<std::string, std::function<std::shared_ptr<AggFun>()>> funVec = {

This comment has been minimized.

Copy link
@monadbobo

monadbobo Aug 14, 2019

Contributor

Maybe using "static constexpr" was more better.


// Init fun handler
if (findIt == data.end()) {
for (auto &col : yieldCols_) {

This comment has been minimized.

Copy link
@monadbobo

monadbobo Aug 14, 2019

Contributor

should use "std::transform" .

for (auto& col : item.second) {
row.emplace_back(col->getResult());
}
rows_.emplace_back();

This comment has been minimized.

Copy link
@monadbobo

monadbobo Aug 14, 2019

Contributor

We can merge these two statements.


// Generate result data
rows_.clear();
for (auto& item : data) {

This comment has been minimized.

Copy link
@monadbobo

monadbobo Aug 14, 2019

Contributor

Replace it with std::transform.

yieldCols_.emplace_back(std::make_pair(col, -1));

if (col->alias() != nullptr) {
if (col->expr()->isInputExpression()) {

This comment has been minimized.

Copy link
@CPWstatic

CPWstatic Sep 10, 2019

Contributor

Should we consider variable?

@laura-ding laura-ding force-pushed the laura-ding:groupby branch from 6393eb4 to 73e07df Sep 16, 2019

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Unit testing passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.