Skip to content

GDS refactor #5033

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

Closed
wants to merge 7 commits into from
Closed

GDS refactor #5033

wants to merge 7 commits into from

Conversation

sdht0
Copy link
Contributor

@sdht0 sdht0 commented Mar 11, 2025

  • Renames the functions to be shorter
  • Uses the new encapsulated arrays.

Contributor agreement

@sdht0 sdht0 requested a review from andyfengHKU March 11, 2025 21:48
void setCoreValue(offset_t offset, degree_t value) {
coreValues[offset].store(value, std::memory_order_relaxed);
}
struct KcoreComputationState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert the naming to CoreValues. Otherwise I find KcoreComputationState.isValid to be very confusing.

class PValues {
public:
PValues(table_id_map_t<offset_t> maxOffsetMap, storage::MemoryManager* mm, double val) {
struct PrComputationState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto revert renaming.

@@ -7,31 +7,31 @@ namespace kuzu {
namespace function {

struct WeaklyConnectedComponentsFunction {
static constexpr const char* name = "WEAKLY_CONNECTED_COMPONENT";
static constexpr const char* name = "GDS_WCC";
Copy link
Contributor

Choose a reason for hiding this comment

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

revert all naming changes in this PR. We should add alias naming instead of renaming.

@@ -87,6 +90,15 @@ class AtomicObjectArray {
}
return false;
}

void compare_and_swap(const common::offset_t pos, double valToAdd) {
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 not what the function is doing. It's an addition using CAS, so call it addCAS.

Also for compare_exchange_strong_max do a similar naming with camel case.

SCCState(const offset_t tableID, const offset_t numNodes, MemoryManager* mm) {
componentIDsMap.allocate(tableID, numNodes, mm);
componentIDs = componentIDsMap.getData(tableID);
struct SccComputationState {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also call this SCCIDs

class ComponentIDs {
public:
ComponentIDs(const table_id_map_t<offset_t>& maxOffsetMap, MemoryManager* mm) {
struct WccComputationState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto revert renaming and I would merge this with SCCIDs. Also why we have Wcc, WCC, Scc, SCC at this same time? Let's use one convention and if we decide to change we change in one PR all together.

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.

2 participants