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

group: support reference vector via deep accessor #1367

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

naoa
Copy link
Member

@naoa naoa commented Jun 8, 2022

No description provided.

lib/group.c Outdated
}
}
group_key_init(ctx, &(data.key), real_key);
group_key_init(ctx, &(data.key), real_key, key);
Copy link
Member

Choose a reason for hiding this comment

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

Should we pass NULL instead of key when key isn't an accessor?

lib/group.c Outdated
a->next->obj && !a->next->next) {
real_key = a->next->obj;
data.need_resolved_id = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify this?

e.g.:

      grn_accessor *a;
      for (a = (grn_accessor *)key;
           a->action == GRN_ACCESSOR_GET_KEY && a->next;
           a = a->next) {
        /* do nothing */
      }
      if (a->next &&
          a->next->action == GRN_ACCESSOR_GET_COLUMN_VALUE &&
          a->next->obj &&
          !a->next->next) {
        real_key = a->next->obj;
        data.need_resolved_id = true;
      }

Copy link
Member Author

Choose a reason for hiding this comment

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

If writing the loop to advance the grn_accessor first, I needed to change the following.

      for (a = (grn_accessor *)accessor;
           a->action == GRN_ACCESSOR_GET_KEY && a->next;
           a = a->next) {
        /* do nothing */
      }
      if (a &&
          a->action == GRN_ACCESSOR_GET_COLUMN_VALUE &&
          a->obj &&
          !a->next) {
        real_key = a->obj;
        data.need_resolved_id = true;
      }

Unlike the before changed condition, it will resolve id even if the first grn_accessor is GRN_ACCESSOR_GET_COLUMN_VALUE, is it ok?(I guess there are no case)

Copy link
Member

Choose a reason for hiding this comment

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

Yes. grn_obj_column() returns a column itself not an accessor for the column with a column name of a table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.
If so, I think it would be easier to understand if it advance the grn_accessor first.

void *key;
grn_table_cursor_get_key(ctx, cursor, &key);
return *((grn_id *)key);
return grn_accessor_resolve_id(ctx, data->key.accessor, id);
Copy link
Member

Choose a reason for hiding this comment

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

This may be slow for no deep case. Can you measure this with no deep case? (I hope that we have continuous benchmark mechanism...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see.
I will try the benchmark later.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!
If this decreases performance, we can use the current logic only for no deep case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I benchmarked with a drilldown of 3,144,060 reference vectors of results that no deep case.
There is a slight speed degradation.
I changed to switch them depends to the depth of the accessor.

Case 1. Use grn_accessor_resolve_id
8.912397623062134 sec
7.918306112289429 sec
7.794296026229858 sec
7.789118528366089 sec
7.804325819015503 sec

Case 2. Use grn_table_cursor_get key
8.694790124893188 sec
7.541107654571533 sec
7.652536869049072 sec
7.514059543609619 sec
7.609168767929077 sec

Case 3. Switch grn_table_cursor_get/grn_accessor_resolve_id depends to the depth of the accessor.
8.73039174079895 sec
7.665851831436157 sec
7.721802234649658 sec
7.707949638366699 sec
7.606190919876099 sec

@naoa naoa force-pushed the group-deep-resolve-weight-reference-vector branch from 255d3a4 to 5d7f16b Compare June 8, 2022 00:57
@kou kou merged commit 33df371 into groonga:master Jun 17, 2022
@kou
Copy link
Member

kou commented Jun 17, 2022

Thanks!

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.

None yet

2 participants