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 create with serial #1812

Merged
merged 1 commit into from
Jul 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 12 additions & 1 deletion src/binder/bind/bind_updating_clause.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,23 @@ std::unique_ptr<BoundCreateNode> Binder::bindCreateNode(
auto primaryKey = nodeTableSchema->getPrimaryKey();
std::shared_ptr<Expression> primaryKeyExpression;
std::vector<expression_pair> setItems;
for (auto& property : catalog.getReadOnlyVersion()->getNodeProperties(nodeTableID)) {
if (collection.hasKeyVal(node, property.name)) {
setItems.emplace_back(collection.getKeyVal(node, property.name));
} else {
auto propertyExpression =
expressionBinder.bindNodePropertyExpression(*node, property.name);
auto nullExpression = expressionBinder.createNullLiteralExpression();
nullExpression = ExpressionBinder::implicitCastIfNecessary(
nullExpression, propertyExpression->dataType);
setItems.emplace_back(std::move(propertyExpression), std::move(nullExpression));
}
}
for (auto& [key, val] : collection.getKeyVals(node)) {
auto propertyExpression = static_pointer_cast<PropertyExpression>(key);
if (propertyExpression->getPropertyID(nodeTableID) == primaryKey.propertyID) {
primaryKeyExpression = val;
}
setItems.emplace_back(key, val);
}
if (nodeTableSchema->getPrimaryKey().dataType.getLogicalTypeID() != LogicalTypeID::SERIAL &&
primaryKeyExpression == nullptr) {
Expand Down
4 changes: 0 additions & 4 deletions src/catalog/catalog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,6 @@ const Property& CatalogContent::getRelProperty(
throw CatalogException("Cannot find rel property " + propertyName + ".");
}

std::vector<Property> CatalogContent::getAllNodeProperties(table_id_t tableID) const {
return nodeTableSchemas.at(tableID)->getAllNodeProperties();
}

void CatalogContent::dropTableSchema(table_id_t tableID) {
auto tableSchema = getTableSchema(tableID);
if (tableSchema->isNodeTable) {
Expand Down
6 changes: 4 additions & 2 deletions src/include/catalog/catalog.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,11 @@ class CatalogContent {
const Property& getRelProperty(
common::table_id_t tableID, const std::string& propertyName) const;

std::vector<Property> getAllNodeProperties(common::table_id_t tableID) const;
inline const std::vector<Property>& getNodeProperties(common::table_id_t tableID) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking if we should enforce a naming style for returning reference of a value, like getNodePropertiesRef, etc. What do you think?

return nodeTableSchemas.at(tableID)->getProperties();
}
inline const std::vector<Property>& getRelProperties(common::table_id_t tableID) const {
return relTableSchemas.at(tableID)->properties;
return relTableSchemas.at(tableID)->getProperties();
}
inline std::vector<common::table_id_t> getNodeTableIDs() const {
std::vector<common::table_id_t> nodeTableIDs;
Expand Down
4 changes: 1 addition & 3 deletions src/include/catalog/catalog_structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ struct TableSchema {
return std::any_of(properties.begin(), properties.end(),
[&propertyName](const Property& property) { return property.name == propertyName; });
}

inline const std::vector<Property>& getProperties() const { return properties; }
inline void addProperty(std::string propertyName, common::LogicalType dataType) {
properties.emplace_back(
std::move(propertyName), std::move(dataType), increaseNextPropertyID(), tableID);
Expand Down Expand Up @@ -102,8 +102,6 @@ struct NodeTableSchema : TableSchema {

inline Property getPrimaryKey() const { return properties[primaryKeyPropertyID]; }

inline std::vector<Property> getAllNodeProperties() const { return properties; }

// TODO(Semih): When we support updating the schemas, we need to update this or, we need
// a more robust mechanism to keep track of which property is the primary key (e.g., store this
// information with the property). This is an idx, not an ID, so as the columns/properties of
Expand Down
7 changes: 6 additions & 1 deletion src/include/storage/storage_structure/column.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class Column : public BaseColumnOrList {
void write(common::ValueVector* nodeIDVector, common::ValueVector* vectorToWriteFrom);

bool isNull(common::offset_t nodeOffset, transaction::Transaction* transaction);
void setNull(common::offset_t nodeOffset);
virtual void setNull(common::offset_t nodeOffset);

inline NullColumn* getNullColumn() { return nullColumn.get(); }

Expand Down Expand Up @@ -175,6 +175,11 @@ class StructPropertyColumn : public Column {
void read(transaction::Transaction* transaction, common::ValueVector* nodeIDVector,
common::ValueVector* resultVector) final;

void setNull(common::offset_t nodeOffset) final;

void write(common::offset_t nodeOffset, common::ValueVector* vectorToWriteFrom,
uint32_t posInVectorToWriteFrom) override;

private:
std::vector<std::unique_ptr<Column>> structFieldColumns;
};
Expand Down
2 changes: 1 addition & 1 deletion src/main/connection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ std::string Connection::getNodePropertyNames(const std::string& tableName) {
auto tableID = catalog->getReadOnlyVersion()->getTableID(tableName);
auto primaryKeyPropertyID =
catalog->getReadOnlyVersion()->getNodeTableSchema(tableID)->getPrimaryKey().propertyID;
for (auto& property : catalog->getReadOnlyVersion()->getAllNodeProperties(tableID)) {
for (auto& property : catalog->getReadOnlyVersion()->getNodeProperties(tableID)) {
result +=
"\t" + property.name + " " + LogicalTypeUtils::dataTypeToString(property.dataType);
result += property.propertyID == primaryKeyPropertyID ? "(PRIMARY KEY)\n" : "\n";
Expand Down
1 change: 1 addition & 0 deletions src/processor/result/factorized_table.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ void FactorizedTable::setOverflowColNull(
// TODO(Guodong): change this function to not use dataChunkPos in ColumnSchema.
uint64_t FactorizedTable::computeNumTuplesToAppend(
const std::vector<ValueVector*>& vectorsToAppend) const {
assert(!vectorsToAppend.empty());
auto unflatDataChunkPos = -1ul;
auto numTuplesToAppend = 1ul;
for (auto i = 0u; i < vectorsToAppend.size(); i++) {
Expand Down
20 changes: 20 additions & 0 deletions src/storage/storage_structure/column.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@

void Column::setNull(common::offset_t nodeOffset) {
nullColumn->setValue(nodeOffset);
auto walPageInfo = createWALVersionOfPageIfNecessaryForElement(nodeOffset, numElementsPerPage);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I think we only need these newly added lines when we are adding a new node, right? Can we change CreateNode to go through Column::write? So we can gradually get rid of setNull interface, whose functionality is overlapped with write.

Let's discuss this a bit more when you are online.

bufferManager->unpin(*wal->fileHandle, walPageInfo.pageIdxInWAL);
fileHandle->releaseWALPageIdxLock(walPageInfo.originalPageIdx);
}

Value Column::readValueForTestingOnly(offset_t offset) {
Expand Down Expand Up @@ -325,6 +328,23 @@
}
}

void StructPropertyColumn::setNull(common::offset_t nodeOffset) {
nullColumn->setValue(nodeOffset);
// TODO(Ziyi): we currently only support create a node/rel with null struct property because
// we don't have a mechanism to log the sub columns of a struct column.
}

void StructPropertyColumn::write(common::offset_t nodeOffset,
common::ValueVector* vectorToWriteFrom, uint32_t posInVectorToWriteFrom) {
bool isNull = vectorToWriteFrom->isNull(posInVectorToWriteFrom);
if (nullColumn) {
nullColumn->write(nodeOffset, vectorToWriteFrom, posInVectorToWriteFrom);
}
if (!isNull) {
throw common::RuntimeException{"Can only update a null value to struct column."};

Check warning on line 344 in src/storage/storage_structure/column.cpp

View check run for this annotation

Codecov / codecov/patch

src/storage/storage_structure/column.cpp#L344

Added line #L344 was not covered by tests
}
}

void InternalIDColumn::readInternalIDsFromPage(transaction::Transaction* transaction,
uint8_t* frame, PageElementCursor& pageCursor, common::ValueVector* resultVector,
uint32_t posInVector, uint32_t numValuesToRead, DiskOverflowFile* diskOverflowFile) {
Expand Down
2 changes: 1 addition & 1 deletion src/storage/store/node_table.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ NodeTable::NodeTable(NodesStatisticsAndDeletedIDs* nodesStatisticsAndDeletedIDs,
std::unordered_map<common::property_id_t, std::unique_ptr<Column>> NodeTable::initializeColumns(
WAL* wal, kuzu::storage::BufferManager* bm, NodeTableSchema* nodeTableSchema) {
std::unordered_map<common::property_id_t, std::unique_ptr<Column>> propertyColumns;
for (auto& property : nodeTableSchema->getAllNodeProperties()) {
for (auto& property : nodeTableSchema->getProperties()) {
propertyColumns[property.propertyID] = ColumnFactory::getColumn(
StorageUtils::getNodePropertyColumnStructureIDAndFName(wal->getDirectory(), property),
property.dataType, bm, wal);
Expand Down
13 changes: 5 additions & 8 deletions test/test_files/tck/match/match1.test
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@
Binder exception: No node table exists in database.

# Matching all nodes
# TODO(Guodong/Ziyi): Fixme
-CASE Scenario2
-SKIP
-STATEMENT CREATE NODE TABLE A(ID SERIAL, name STRING, PRIMARY KEY(ID));
---- ok
-STATEMENT CREATE NODE TABLE B(ID SERIAL, name STRING, PRIMARY KEY(ID));
Expand All @@ -21,13 +19,11 @@ Binder exception: No node table exists in database.
---- ok
-STATEMENT MATCH (n) RETURN n;
---- 2
1
2
{_ID: 0:0, _LABEL: A, ID: 0}
{_ID: 1:0, _LABEL: B, ID: 0, name: b}

# Matching nodes using multiple labels
# TODO(Guodong/Ziyi): Fixme
-CASE Scenario3
-SKIP
-STATEMENT CREATE NODE TABLE A(ID SERIAL, PRIMARY KEY(ID));
---- ok
-STATEMENT CREATE NODE TABLE B(ID SERIAL, PRIMARY KEY(ID));
Expand All @@ -37,8 +33,9 @@ Binder exception: No node table exists in database.
-STATEMENT CREATE (:A), (:B), (:C);
---- ok
-STATEMENT MATCH (a:A:B) RETURN a;
---- 1
1
---- 2
{_ID: 0:0, _LABEL: A, ID: 0}
{_ID: 1:0, _LABEL: B, ID: 0}

# Simple node inlnie property predicate
-CASE Scenario4
Expand Down
Loading