Skip to content

feat: add transaction related method - #360

Closed
hantmac wants to merge 13 commits into
databendlabs:mainfrom
hantmac:feat/add-transaction-method
Closed

feat: add transaction related method#360
hantmac wants to merge 13 commits into
databendlabs:mainfrom
hantmac:feat/add-transaction-method

Conversation

@hantmac

@hantmac hantmac commented Mar 6, 2024

Copy link
Copy Markdown
Member
  • Add begin, commit, rollback transaction method.

@hantmac

hantmac commented Mar 12, 2024

Copy link
Copy Markdown
Member Author

So strange, I run cargo clippy --all-targets -- -D warnings successfully on local machine, but the ci failed https://github.com/datafuselabs/bendsql/actions/runs/8242790623/job/22542294683?pr=360#step:4:586

@hantmac
hantmac force-pushed the feat/add-transaction-method branch from 48d0780 to 17da5f4 Compare March 12, 2024 02:47
Comment thread driver/src/flight_sql.rs Outdated
Comment thread driver/src/conn.rs Outdated
Comment thread sql/src/value.rs Outdated
@hantmac
hantmac force-pushed the feat/add-transaction-method branch from 78d279f to 60a1b79 Compare March 19, 2024 06:47
Comment thread driver/src/conn.rs Outdated
}

async fn begin(&self) -> Result<()> {
let _ = self.exec("BEGIN").await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

errors should be handled?

@hantmac
hantmac force-pushed the feat/add-transaction-method branch from 2be8397 to f6e3db8 Compare March 21, 2024 03:09
let (val,): (i32,) = row.try_into().unwrap();
assert_eq!(val, 1);

conn.rollback().await.unwrap();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should recheck after rollback?

@sundy-li

sundy-li commented Sep 9, 2026

Copy link
Copy Markdown
Member

Review result: closing this PR.

Reason: This PR has not been updated for a long time and is currently in conflict with main (CONFLICTING/DIRTY). The existing implementation also left review comments about transaction error handling and rollback verification unresolved, so it does not form a mergeable final version. If transaction support is still needed, please submit a new PR based on the current Connection/IConnection structure, with proper error propagation for REST/FlightSQL and dedicated tests for commit and rollback behavior. Thanks for the contribution; feel free to reopen the discussion or submit a new PR.

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.

3 participants