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

fix(client): kneigbor-api has unmatched edge type with server #644

Closed

Conversation

LiJie20190102
Copy link

@LiJie20190102 LiJie20190102 commented Nov 26, 2024

When calling the 'kneigbor' interface and returning the result, scenarios where 'withPath' is' true 'and' false 'are incompatible (#643)

Purpose of the PR

Main Changes

Due to the different results returned by the server-side code (org.apache.hugegraph.api.traversers.KneighborAPI#post), the org.apache.hugegraph.structure.traverser.Kneighbor file has been modified

image

For example:

  • If you introduce a new feature, please show detailed design here or add the link of design documentation.
  • If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
  • If there is a discussion in the mailing list, please add the link. -->

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

lijie0203 added 2 commits November 26, 2024 14:12
…result, scenarios where 'withPath' is' true 'and' false 'are incompatible (apache#43)
…result, scenarios where 'withPath' is' true 'and' false 'are incompatible (apache#643)
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Nov 26, 2024
@github-actions github-actions bot added the client hugegraph-client label Nov 26, 2024
@dosubot dosubot bot added the bug Something isn't working label Nov 26, 2024
@LiJie20190102
Copy link
Author

@imbajin @VGalaxies Please help review it

@@ -37,7 +37,7 @@ public class Kneighbor {
@JsonProperty
private Set<Vertex> vertices;
@JsonProperty
private Set<Edge> edges;
private Set<?> edges;
Copy link
Member

@imbajin imbajin Nov 26, 2024

Choose a reason for hiding this comment

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

Although using <?> may take effect(no add/update operation here), it is still strange why the restriction of generics has been modified to arbitrary here.

It is possible that the server-side changes did not meet expectations?

Wanna to know in what case do I need to pass in a non-Edge Edge?(Is it because the definition of server-side generics is too broad?)

Copy link
Author

Choose a reason for hiding this comment

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

server code
org.apache.hugegraph.api.traversers.KneighborAPI#post
image

Copy link
Member

Choose a reason for hiding this comment

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

server code org.apache.hugegraph.api.traversers.KneighborAPI#post image

I mean the change in server maybe not suitable.

A better approach could be to adjust the <> type of server? You can take a look at it
image

Copy link
Author

@LiJie20190102 LiJie20190102 Nov 27, 2024

Choose a reason for hiding this comment

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

How about this:If 'with_idge' is' false ', we will not return any information about the edge; If 'with-vertex' is' false 'and does not return vertex information, the' kneigbor 'parameter has already returned a set of vertex IDs
image

Copy link
Member

Choose a reason for hiding this comment

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

ow about this:If 'with_idge' is' false ', we will not return any information about the edge; If 'with-vertex' is' false 'and does not return vertex information, the' kneigbor 'parameter has already returned a set of vertex IDs

Looks okay, but what effect does the iterator returned earlier? Do you have to test what the potential effect is there? See if the test can pass rightly

Copy link
Author

@LiJie20190102 LiJie20190102 Nov 27, 2024

Choose a reason for hiding this comment

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

ow about this:If 'with_idge' is' false ', we will not return any information about the edge; If 'with-vertex' is' false 'and does not return vertex information, the' kneigbor 'parameter has already returned a set of vertex IDs

Looks okay, but what effect does the iterator returned earlier? Do you have to test what the potential effect is there? See if the test can pass rightly

if with_edge is false:
{ "kneighbor": [ "2:car1!2", "2:car2!4" ], "size": 2, "paths": [ { "objects": [ "1:zhangsan", "2:car1!2" ] }, { "objects": [ "1:zhangsan", "2:car2!4" ] } ], "vertices": [ { "id": "2:car2!4", "label": "car", "type": "vertex", "properties": { "age": 4, "qsdi_task_id": "car2_task_id_test", "qsdi_record_id": "test_record_id", "name": "car2", "qsdi_data_source": "1" } }, { "id": "1:zhangsan", "label": "people", "type": "vertex", "properties": { "age": 55, "qsdi_record_id": "926133154860048384", "name": "zhangsan", "qsdi_data_source": "1" } }, { "id": "2:car1!2", "label": "car", "type": "vertex", "properties": { "age": 2, "qsdi_task_id": "car1_task_id_test", "qsdi_record_id": "car1_log_id_test", "name": "car1", "qsdi_data_source": "1" } } ], "edges": [ "S1:zhangsan>3>3>>S2:car2!4", "S1:zhangsan>3>3>>S2:car1!2" ] }

if with_edge is true:
{ "kneighbor": [ "2:car1!2", "2:car2!4" ], "size": 2, "paths": [ { "objects": [ "1:zhangsan", "2:car1!2" ] }, { "objects": [ "1:zhangsan", "2:car2!4" ] } ], "vertices": [ { "id": "2:car2!4", "label": "car", "type": "vertex", "properties": { "age": 4, "qsdi_task_id": "car2_task_id_test", "qsdi_record_id": "test_record_id", "name": "car2", "qsdi_data_source": "1" } }, { "id": "1:zhangsan", "label": "people", "type": "vertex", "properties": { "age": 55, "qsdi_record_id": "926133154860048384", "name": "zhangsan", "qsdi_data_source": "1" } }, { "id": "2:car1!2", "label": "car", "type": "vertex", "properties": { "age": 2, "qsdi_task_id": "car1_task_id_test", "qsdi_record_id": "car1_log_id_test", "name": "car1", "qsdi_data_source": "1" } } ], "edges": [ { "id": "S1:zhangsan>3>3>>S2:car2!4", "label": "driver_single", "type": "edge", "outV": "1:zhangsan", "outVLabel": "people", "inV": "2:car2!4", "inVLabel": "car", "properties": { "qsdi_task_id": "zhangsan_car2_rask_id_test", "qsdi_record_id": "926133802515111937", "name": "ride12", "driver_count": 23, "qsdi_data_source": "1" } }, { "id": "S1:zhangsan>3>3>>S2:car1!2", "label": "driver_single", "type": "edge", "outV": "1:zhangsan", "outVLabel": "people", "inV": "2:car1!2", "inVLabel": "car", "properties": { "qsdi_record_id": "926133804008284160", "name": "ride88", "driver_count": 88, "qsdi_data_source": "1" } } ] }

Copy link
Member

Choose a reason for hiding this comment

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

THX, could try to submit a PR in server (or in your own forked repo to test the CI)

BTW, test/ in local env is also fine:)

Copy link
Contributor

Choose a reason for hiding this comment

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

after apache/incubator-hugegraph#2699 merged, we don't need to change Set<Edge> edges?

@imbajin imbajin changed the title fix(client): When calling the 'kneigbor' interface and returning the … fix(client): kneigbor-api has unmatched edge type with server Nov 26, 2024
@imbajin imbajin requested a review from javeme November 26, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client hugegraph-client size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
3 participants