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

integrate falkordbrs #544

Merged
merged 59 commits into from
Mar 3, 2024
Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
351a39f
integrate falkordbrs
AviAvni Dec 25, 2023
41a7a6f
fix submodule
AviAvni Dec 25, 2023
0ba578a
fix
AviAvni Dec 25, 2023
5358d33
fix
AviAvni Dec 25, 2023
2175df2
separate build for debug
AviAvni Dec 26, 2023
72e119b
build rust with sanitizer flags when needed
AviAvni Dec 26, 2023
1805a0f
add workspace and update
AviAvni Dec 26, 2023
7000d6b
fix leak
AviAvni Dec 26, 2023
000dd3e
update
AviAvni Dec 26, 2023
2ff1098
address review
AviAvni Dec 26, 2023
baa42f6
use alpine image in sanitizer
AviAvni Dec 26, 2023
3ac957a
move to ubuntu
AviAvni Dec 26, 2023
7e6879b
enable cargo
AviAvni Dec 26, 2023
11e6103
fix
AviAvni Dec 26, 2023
a48284f
fix
AviAvni Dec 26, 2023
1aedfd4
fix
AviAvni Dec 26, 2023
4b85282
fix
AviAvni Dec 27, 2023
cf582b7
update
AviAvni Dec 27, 2023
3bb4e95
use target dir
AviAvni Dec 27, 2023
5316712
addres review
AviAvni Dec 27, 2023
5536be5
use nightly rust in sanitizer
AviAvni Dec 27, 2023
e9b4083
address review
AviAvni Dec 27, 2023
25a0d51
fix
AviAvni Dec 27, 2023
3022533
fix
AviAvni Dec 27, 2023
4186542
update build
AviAvni Dec 27, 2023
be795a2
fix
AviAvni Dec 27, 2023
bd07f6c
address review
AviAvni Dec 27, 2023
c7d67da
build
AviAvni Dec 27, 2023
06fccfe
update
AviAvni Dec 27, 2023
830b2d1
update
AviAvni Dec 27, 2023
d9bc166
fix build
AviAvni Dec 27, 2023
d1bd8e4
update
AviAvni Dec 27, 2023
3a3d2d4
fix codeql and address review
AviAvni Dec 27, 2023
03adde8
address review
AviAvni Dec 27, 2023
a8a6cdf
add alpine
AviAvni Dec 27, 2023
9a32839
update for alpine
AviAvni Dec 28, 2023
eaa1676
update
AviAvni Dec 28, 2023
34441cb
fix build
AviAvni Dec 28, 2023
8af4d62
remove debian
AviAvni Dec 31, 2023
b78fa3c
update
AviAvni Dec 31, 2023
72a0455
update
AviAvni Dec 31, 2023
d3fbff7
use current headers instead of generated one
AviAvni Jan 2, 2024
235b72e
clean
AviAvni Jan 2, 2024
01c1cbb
fix for mac
AviAvni Jan 18, 2024
2428bed
document alloc funtion
AviAvni Feb 19, 2024
8223940
Merge branch 'master' into integrate-falkordbrs
AviAvni Feb 19, 2024
af831f4
move to ubuntu image
AviAvni Feb 19, 2024
076688e
change docker tag
AviAvni Feb 19, 2024
eaebc6d
Merge branch 'master' into integrate-falkordbrs
AviAvni Feb 22, 2024
a945af2
update to latest
AviAvni Feb 22, 2024
c976699
update format
AviAvni Feb 22, 2024
d90c004
revert
AviAvni Feb 22, 2024
2375432
fix leak
AviAvni Feb 25, 2024
b407651
always compile rust
AviAvni Feb 25, 2024
7cb2ddf
fix makefile
AviAvni Feb 25, 2024
abdd3a8
review
AviAvni Feb 25, 2024
446424b
address review
AviAvni Mar 3, 2024
917bfac
Merge branch 'master' into integrate-falkordbrs
AviAvni Mar 3, 2024
94c3bc3
address review
AviAvni Mar 3, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ concurrency:
jobs:
build:
runs-on: ubuntu-latest
container: falkordb/falkordb-build:latest
container: falkordb/falkordb-build:ubuntu
services:
registry:
image: registry:2
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
analyze:
name: Analyze
runs-on: ubuntu-latest
container: falkordb/falkordb-build:latest
container: falkordb/falkordb-build:ubuntu
permissions:
actions: read
contents: read
Expand Down Expand Up @@ -88,6 +88,7 @@ jobs:
# If this step fails, then you should remove it and run the build manually (see below)
- name: Build
run: |
rustup default stable
make build

# ℹ️ Command-line programs to run using the OS shell.
Expand Down
7 changes: 5 additions & 2 deletions .github/workflows/sanitize.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ on:
jobs:
sanitize-test:
runs-on: ubuntu-latest
container: falkordb/falkordb-build:latest
container: falkordb/falkordb-build:ubuntu
steps:

- name: Safe dir
Expand Down Expand Up @@ -40,7 +40,10 @@ jobs:
key: search-sanitizer-${{ hashFiles('./deps/RediSearch/src/version.h') }}

- name: Build
run: make CLANG=1 SAN=address NPROC=16
run: |
rustup toolchain list
rustup default nightly
make CLANG=1 SAN=address NPROC=16
Copy link

Choose a reason for hiding this comment

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

The addition of Rust toolchain management commands (rustup toolchain list and rustup default nightly) before the build command is a crucial step for ensuring that the correct Rust version is used for the build. This is especially important when working with Rust, where nightly features might be required. However, it's recommended to document why the nightly toolchain is necessary for this project, as using nightly features can affect the stability and portability of the code.

Consider adding comments in the workflow file explaining why the nightly toolchain is selected and any implications this might have.


- name: Unit tests
id: unit_tests
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,5 @@ deps/GraphBLAS/Config/GraphBLAS.h.tmp
/venv/
/.vscode/
/1/

target
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,6 @@
[submodule "deps/oniguruma"]
path = deps/oniguruma
url = https://github.com/kkos/oniguruma.git
[submodule "deps/FalkorDB-rs"]
path = deps/FalkorDB-rs
url = https://github.com/FalkorDB/FalkorDB-rs.git
5 changes: 3 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ include_directories(
${root}/deps/xxHash
${root}/deps/utf8proc
${root}/deps/oniguruma
$ENV{FalkorDBRS_BINDIR}
AviAvni marked this conversation as resolved.
Show resolved Hide resolved
${root}/deps/RediSearch/src
${root}/deps/GraphBLAS/Include
${root}/deps/libcypher-parser/lib/src
Expand Down Expand Up @@ -52,8 +53,8 @@ set(FALKORDB_OBJECTS $<TARGET_OBJECTS:falkordb>)

find_package(OpenSSL)

lists_from_env(GRAPHBLAS LIBXXHASH RAX LIBCYPHER_PARSER REDISEARCH_LIBS UTF8PROC ONIGURUMA)
set(FALKORDB_LIBS ${GRAPHBLAS} ${LIBXXHASH} ${RAX} ${LIBCYPHER_PARSER} ${REDISEARCH_LIBS} ${UTF8PROC} ${ONIGURUMA} OpenSSL::SSL)
lists_from_env(GRAPHBLAS LIBXXHASH RAX LIBCYPHER_PARSER REDISEARCH_LIBS UTF8PROC ONIGURUMA FalkorDBRS)
set(FALKORDB_LIBS ${GRAPHBLAS} ${LIBXXHASH} ${RAX} ${LIBCYPHER_PARSER} ${REDISEARCH_LIBS} ${UTF8PROC} ${ONIGURUMA} ${FalkorDBRS} OpenSSL::SSL)
Comment on lines +55 to +56
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The script's execution confirmed that the libFalkorDB_rs.a file, necessary for linking FalkorDBRS, was not found. This indicates an issue with either the build process of FalkorDBRS or the setup of the FalkorDBRS_BINDIR environment variable, which is supposed to point to the directory containing the library. It's crucial to ensure that FalkorDBRS is correctly built and that the FalkorDBRS_BINDIR environment variable is accurately set to the directory where libFalkorDB_rs.a is located. Addressing this issue is necessary for a successful build and linking process.

Analysis chain

The addition of FalkorDBRS to FALKORDB_LIBS is noted. However, based on previous comments, there seems to be an issue with finding the libFalkorDB_rs.a file, indicating that FalkorDBRS might not be properly built or located. It's crucial to ensure that FalkorDBRS is correctly built and available in the expected directory before proceeding with the linking process. This might involve verifying the build steps for FalkorDBRS or adjusting the search paths to locate the library file correctly.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify the build and availability of the FalkorDBRS library
if [ -f "$FalkorDBRS_BINDIR/libFalkorDB_rs.a" ]; then
    echo "FalkorDBRS library found and ready for linking."
else
    echo "FalkorDBRS library not found. Please check the build process and directory paths."
    exit 1
fi

Length of output: 212


target_link_options(falkordb PRIVATE ${CMAKE_LD_FLAGS_LIST} ${CMAKE_SO_LD_FLAGS_LIST})
target_link_libraries(falkordb PRIVATE ${FALKORDB_LIBS} ${CMAKE_LD_LIBS})
Expand Down
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[workspace]
resolver = "2"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of this line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

members = [
"deps/FalkorDB-rs",
]
33 changes: 29 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,13 @@ REDISEARCH_DIR = $(ROOT)/deps/RediSearch
export REDISEARCH_BINROOT=$(BINROOT)
include $(ROOT)/build/RediSearch/Makefile.defs

FalkorDBRS_DIR = $(ROOT)/deps/FalkorDB-rs
export FalkorDBRS_BINDIR=$(BINROOT)/FalkorDB-rs
include $(ROOT)/build/FalkorDB-rs/Makefile.defs

BIN_DIRS += $(REDISEARCH_BINROOT)/search-static

LIBS=$(RAX) $(LIBXXHASH) $(GRAPHBLAS) $(REDISEARCH_LIBS) $(LIBCYPHER_PARSER) $(UTF8PROC) $(ONIGURUMA)
LIBS=$(RAX) $(LIBXXHASH) $(GRAPHBLAS) $(REDISEARCH_LIBS) $(LIBCYPHER_PARSER) $(UTF8PROC) $(ONIGURUMA) $(FalkorDBRS)

#----------------------------------------------------------------------------------------------

Expand Down Expand Up @@ -189,11 +193,15 @@ ifneq ($(call files_missing,$(REDISEARCH_LIBS)),)
MISSING_DEPS += $(REDISEARCH_LIBS)
endif

ifeq ($(wildcard $(FalkorDBRS)),)
MISSING_DEPS += $(FalkorDBRS)
endif

ifneq ($(MISSING_DEPS),)
DEPS=1
endif

DEPENDENCIES=libcypher-parser graphblas redisearch rax libxxhash utf8proc oniguruma
DEPENDENCIES=libcypher-parser graphblas redisearch rax libxxhash utf8proc oniguruma falkordbrs

ifneq ($(filter all deps $(DEPENDENCIES) pack,$(MAKECMDGOALS)),)
DEPS=1
Expand All @@ -219,7 +227,7 @@ include $(MK)/rules

ifeq ($(DEPS),1)

deps: $(LIBCYPHER_PARSER) $(GRAPHBLAS) $(LIBXXHASH) $(RAX) $(REDISEARCH_LIBS) $(UTF8PROC) $(ONIGURUMA)
deps: $(LIBCYPHER_PARSER) $(GRAPHBLAS) $(LIBXXHASH) $(RAX) $(REDISEARCH_LIBS) $(UTF8PROC) $(ONIGURUMA) $(FalkorDBRS)

libxxhash: $(LIBXXHASH)

Expand Down Expand Up @@ -265,7 +273,24 @@ $(REDISEARCH_LIBS):
@echo Building $@ ...
$(SHOW)$(MAKE) -C $(REDISEARCH_DIR) STATIC=1 BINROOT=$(REDISEARCH_BINROOT) CC=$(CC) CXX=$(CXX)

.PHONY: libcypher-parser graphblas redisearch libxxhash rax utf8proc oniguruma
falkordbrs: $(FalkorDBRS)

ifeq ($(DEBUG),1)
CARGO_FLAGS=
else
CARGO_FLAGS=--release
endif
AviAvni marked this conversation as resolved.
Show resolved Hide resolved

ifneq ($(SAN),)
export RUSTFLAGS=-Zsanitizer=$(SAN)
CARGO_FLAGS=--target x86_64-unknown-linux-gnu
endif
AviAvni marked this conversation as resolved.
Show resolved Hide resolved

$(FalkorDBRS):
@echo Building $@ ...
cd deps/FalkorDB-rs && cargo build $(CARGO_FLAGS) --features falkordb_allocator --target-dir $(FalkorDBRS_BINDIR)

.PHONY: libcypher-parser graphblas redisearch libxxhash rax utf8proc oniguruma falkordbrs

#----------------------------------------------------------------------------------------------

Expand Down
10 changes: 10 additions & 0 deletions build/FalkorDB-rs/Makefile.defs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

ifneq ($(SAN),)
export FalkorDBRS=$(FalkorDBRS_BINDIR)/x86_64-unknown-linux-gnu/debug/libFalkorDB_rs.a
AviAvni marked this conversation as resolved.
Show resolved Hide resolved
else
ifeq ($(DEBUG),1)
export FalkorDBRS=$(FalkorDBRS_BINDIR)/debug/libFalkorDB_rs.a
else
export FalkorDBRS=$(FalkorDBRS_BINDIR)/release/libFalkorDB_rs.a
endif
endif
2 changes: 1 addition & 1 deletion build/docker/Dockerfile.compiler
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
ARG TARGETPLATFORM=linux/amd64

FROM --platform=$TARGETPLATFORM falkordb/falkordb-build:latest as builder
FROM --platform=$TARGETPLATFORM falkordb/falkordb-build:ubuntu as builder

WORKDIR /FalkorDB

Expand Down
29 changes: 29 additions & 0 deletions build/docker/Dockerfile.debian
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
FROM --platform=$TARGETPLATFORM falkordb/falkordb-build:debian as build

ARG TARGETPLATFORM

WORKDIR /FalkorDB

ADD . /FalkorDB

RUN make
Copy link

Choose a reason for hiding this comment

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

As with the Ubuntu Dockerfile, it's recommended to specify the make target explicitly.

- RUN make
+ RUN make all

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
RUN make
RUN make all


FROM --platform=$TARGETPLATFORM redis:7.2.3-bookworm

ARG TARGETPLATFORM

COPY --from=build /FalkorDB/bin /FalkorDB/bin
COPY --from=build /FalkorDB/build/docker/run.sh /FalkorDB/build/docker/run.sh

ENV ARCH=${TARGETPLATFORM}

# install dependencies
RUN apt-get update -y && \
apt-get install libgomp1 -y

EXPOSE 6379/tcp

ENV FALKORDB_ARGS="MAX_QUEUED_QUERIES 25 TIMEOUT 1000 RESULTSET_SIZE 10000"

CMD /FalkorDB/build/docker/run.sh

File renamed without changes.
1 change: 1 addition & 0 deletions deps/FalkorDB-rs
Submodule FalkorDB-rs added at e05697
2 changes: 1 addition & 1 deletion deps/RediSearch
2 changes: 1 addition & 1 deletion src/arithmetic/arithmetic_expression.c
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ AR_ExpNode *AR_EXP_NewAttributeAccessNode(AR_ExpNode *entity,
GraphContext *gc = QueryCtx_GetGraphCtx();
SIValue prop_idx = SI_LongVal(ATTRIBUTE_ID_NONE);
SIValue prop_name = SI_ConstStringVal((char *)attr);
Attribute_ID idx = GraphContext_GetAttributeID(gc, attr);
AttributeID idx = GraphContext_GetAttributeID(gc, attr);
AviAvni marked this conversation as resolved.
Show resolved Hide resolved

if(idx != ATTRIBUTE_ID_NONE) prop_idx = SI_LongVal(idx);

Expand Down
2 changes: 1 addition & 1 deletion src/arithmetic/entity_funcs/entity_funcs.c
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ SIValue AR_PROPERTY(SIValue *argv, int argc, void *private_data) {
// retrieve entity property
GraphEntity *graph_entity = (GraphEntity *)obj.ptrval;
const char *prop_name = argv[1].stringval;
Attribute_ID prop_idx = argv[2].longval;
AttributeID prop_idx = argv[2].longval;

// We have the property string, attempt to look up the index now.
if(prop_idx == ATTRIBUTE_ID_NONE) {
Expand Down
8 changes: 4 additions & 4 deletions src/bulk_insert/bulk_insert.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ static int* _BulkInsert_ReadHeaderLabels
}

// read the property keys from a header
static Attribute_ID* _BulkInsert_ReadHeaderProperties
static AttributeID* _BulkInsert_ReadHeaderProperties
(
GraphContext* gc,
SchemaType t,
Expand All @@ -105,7 +105,7 @@ static Attribute_ID* _BulkInsert_ReadHeaderProperties

if (*prop_count == 0) return NULL;

Attribute_ID* prop_indices = rm_malloc(*prop_count * sizeof(Attribute_ID));
AttributeID* prop_indices = rm_malloc(*prop_count * sizeof(AttributeID));

// the rest of the line is [char *prop_key] * prop_count
for (uint j = 0; j < *prop_count; j++) {
Expand Down Expand Up @@ -207,7 +207,7 @@ static int _BulkInsert_ProcessNodeFile
int* label_ids = _BulkInsert_ReadHeaderLabels(gc, SCHEMA_NODE, data, &data_idx);
uint label_count = array_len(label_ids);
// read the CSV header properties and collect their indices
Attribute_ID* prop_indices = _BulkInsert_ReadHeaderProperties(gc, SCHEMA_NODE, data,
AttributeID* prop_indices = _BulkInsert_ReadHeaderProperties(gc, SCHEMA_NODE, data,
&data_idx, &prop_count);

// sync each matrix once
Expand Down Expand Up @@ -266,7 +266,7 @@ static int _BulkInsert_ProcessEdgeFile
ASSERT(type_count == 1);

int type_id = type_ids[0];
Attribute_ID* prop_indices = _BulkInsert_ReadHeaderProperties(gc, SCHEMA_EDGE,
AttributeID* prop_indices = _BulkInsert_ReadHeaderProperties(gc, SCHEMA_EDGE,
data, &data_idx, &prop_count);

// sync matrix once
Expand Down
17 changes: 8 additions & 9 deletions src/commands/cmd_constraint.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "../query_ctx.h"
#include "../index/indexer.h"
#include "../graph/graph_hub.h"
#include "../undo_log/undo_log.h"
AviAvni marked this conversation as resolved.
Show resolved Hide resolved
#include "../graph/graphcontext.h"
#include "constraint/constraint.h"

Expand All @@ -21,13 +20,13 @@ typedef enum {
CT_DROP // drop constraint
} ConstraintOp;

static inline int _cmp_Attribute_ID
static inline int _cmp_AttributeID
(
const void *a,
const void *b
) {
const Attribute_ID *_a = a;
const Attribute_ID *_b = b;
const AttributeID *_a = a;
const AttributeID *_b = b;
return *_a - *_b;
}

Expand Down Expand Up @@ -146,7 +145,7 @@ static bool _Constraint_Drop
const char **props // properties
) {
bool res = true; // optimistic
Attribute_ID attrs[n];
AttributeID attrs[n];

//--------------------------------------------------------------------------
// try to get graph
Expand Down Expand Up @@ -179,7 +178,7 @@ static bool _Constraint_Drop
const char *prop = props[i];

// try to get property ID
Attribute_ID id = GraphContext_GetAttributeID(gc, prop);
AttributeID id = GraphContext_GetAttributeID(gc, prop);

if(id == ATTRIBUTE_ID_NONE) {
// attribute missing
Expand Down Expand Up @@ -261,7 +260,7 @@ static bool _Constraint_Create
// convert attribute name to attribute ID
//--------------------------------------------------------------------------

Attribute_ID attr_ids[n];
AttributeID attr_ids[n];
for(uint i = 0; i < n; i++) {
attr_ids[i] = FindOrAddAttribute(gc, props[i], true);
}
Expand All @@ -272,7 +271,7 @@ static bool _Constraint_Create

// sort the properties for an easy comparison later
bool dups = false;
qsort(attr_ids, n, sizeof(Attribute_ID), _cmp_Attribute_ID);
qsort(attr_ids, n, sizeof(AttributeID), _cmp_AttributeID);
for(uint i = 0; i < n - 1; i++) {
if(attr_ids[i] == attr_ids[i+1]) {
dups = true;
Expand All @@ -291,7 +290,7 @@ static bool _Constraint_Create
// must be aligned with attribute names array
for(uint i = 0; i < n; i++) {
// get attribute id for attribute name
Attribute_ID attr_id = attr_ids[i];
AttributeID attr_id = attr_ids[i];

// update props to hold graph context's attribute name
props[i] = GraphContext_GetAttributeString(gc, attr_id);
Expand Down
2 changes: 1 addition & 1 deletion src/commands/index_operations.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ static bool index_delete
//--------------------------------------------------------------------------

// quickly return if attribute doesn't exist
Attribute_ID attr_id = GraphContext_GetAttributeID(gc, attr);
AttributeID attr_id = GraphContext_GetAttributeID(gc, attr);
if(attr_id == ATTRIBUTE_ID_NONE) {
ErrorCtx_SetError(EMSG_UNABLE_TO_DROP_INDEX, lbl, attr);
return false;
AviAvni marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading
Loading