Skip to content

Conversation

@markgomer
Copy link
Contributor

@markgomer markgomer commented Feb 2, 2023

On src/backend/utils/graph_generation.c:
Added function to create a Barbell Graph

  • Modified function create_complete_graph to return the Oid
    of the created graph;
  • Created auxiliary function to get_next_val_internal;
  • Currently creates a bridge of no vertexes between graphs,
    will be implemented next.

On age--1.2.0.sql:
Reflect modifications in graph_generation.c

  • Changed return type of create_complete_graph;
  • Added signature to age_create_barbell_graph;

Resolves #275;

@jrgemignani
Copy link
Contributor

@markgomer

Please try to verify that your PRs successfully build, without errors, before submitting them; this PR failed. If it did successfully build, please check that your build environment is set up correctly.

Additionally, make sure that you follow our coding standard https://age.apache.org/contribution/guide/#code-style-guilde

@markgomer markgomer force-pushed the barbell-graph-generation branch from 34ca1af to 4513a34 Compare February 3, 2023 13:56
* Created auxiliary function to get_next_val_internal;
* Currently creates a bridge of no vertexes between graphs,
will be implemented next.
@markgomer markgomer force-pushed the barbell-graph-generation branch from 4513a34 to 654dadf Compare February 3, 2023 13:59
@markgomer
Copy link
Contributor Author

Turns out that I had changed the return type of the function create_complete_graph from void to Oid, and it caused differences in graph_generation.out on the regress test.

I changed the return type of the functions to void as it was not really necessary to have it as Oid.

@markgomer markgomer marked this pull request as ready for review February 3, 2023 15:10
@jrgemignani
Copy link
Contributor

@markgomer

Additionally, make sure that you follow our coding standard https://age.apache.org/contribution/guide/#code-style-guilde

@jrgemignani
Copy link
Contributor

@markgomer Please verify that your code follows our coding standards.

@markgomer
Copy link
Contributor Author

@jrgemignani I don't see where the code doesn't match the standards. I've read the documentation through. Could you point me to what I have to fix?

@jrgemignani
Copy link
Contributor

@jrgemignani I don't see where the code doesn't match the standards. I've read the documentation through. Could you point me to what I have to fix?

There are minor indentation issues and a few if/else statements that are missing braces {}. All if/else statements are required to have braces, even if there is only one line. I know that it is a bit confusing as some of the code doesn't follow that standard, but that code predates the adoption of braces for all if/else statements.

@jrgemignani
Copy link
Contributor

@markgomer Additionally, PG_FUNCTION_INFO_V1 is incorrectly placed. There are 2, one from this commit, the other not. If you could please fix both, that would be great.

* moved PG_FUNCTION_INFO_V1(age_create_barbell_graph) next to
  age_create_barbell_graph function;
* added brackets to if/else conditions if just one line;
* moved get_nextval_internal auxiliary function to the beggining of
  the file.
@markgomer
Copy link
Contributor Author

@jrgemignani, thank you for your time. I have fixed what you pointed. Please let me know if there's anything else left for fixing.

@markgomer
Copy link
Contributor Author

I'll investigate the issue with the test. I'm not sure what happened, since I didn't change anything that would break the code since the last test which passed.

#include "utils/load/ag_load_labels.h"


PG_FUNCTION_INFO_V1(create_complete_graph);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrectly located - it needs to precede the function, not be at the top of the file.



PG_FUNCTION_INFO_V1(create_complete_graph);
PG_FUNCTION_INFO_V1(age_create_barbell_graph);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrectly located - it needs to precede the function, not be at the top of the file.

@jrgemignani jrgemignani merged commit 7628c85 into apache:master Feb 17, 2023
@jrgemignani
Copy link
Contributor

@markgomer I overlooked that you need to have regression tests created to test this feature and added to the PR. Could you please create a PR for regression tests for this feature.

@markgomer
Copy link
Contributor Author

@markgomer I overlooked that you need to have regression tests created to test this feature and added to the PR. Could you please create a PR for regression tests for this feature.

Of course @jrgemignani, I'm already working on that.

@jrgemignani
Copy link
Contributor

@markgomer Thank you! Next time, though, the regression tests need to be included in the PR with the feature or bug fix.

@markgomer markgomer deleted the barbell-graph-generation branch February 17, 2023 19:51
jrgemignani pushed a commit that referenced this pull request Mar 20, 2023
* Added function to create a Barbell Graph

* Created auxiliary function to get_next_val_internal;
* Currently creates a bridge of no vertexes between graphs,
will be implemented next.

* Added signature to age_create_barbell_graph

* Moved bracket to next line to macth code standards

* Fixed patterns to match code standards

* moved PG_FUNCTION_INFO_V1(age_create_barbell_graph) next to
  age_create_barbell_graph function;
* added brackets to if/else conditions if just one line;
* moved get_nextval_internal auxiliary function to the beggining of
  the file.
jrgemignani added a commit that referenced this pull request Mar 24, 2023
* Added function to create a Barbell Graph

* Created auxiliary function to get_next_val_internal;
* Currently creates a bridge of no vertexes between graphs,
will be implemented next.

* Added signature to age_create_barbell_graph

* Moved bracket to next line to macth code standards

* Fixed patterns to match code standards

* moved PG_FUNCTION_INFO_V1(age_create_barbell_graph) next to
  age_create_barbell_graph function;
* added brackets to if/else conditions if just one line;
* moved get_nextval_internal auxiliary function to the beggining of
  the file.

Conflicts:
	src/backend/utils/graph_generation.c

Co-authored-by: John Gemignani <jrgemignani@gmail.com>
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.

Barbell Graph

2 participants