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 #40 rows.Next() skips first records #41

Closed

Conversation

simulot
Copy link
Contributor

@simulot simulot commented Nov 26, 2020

Fix and simplify (DataSet) Next function

I suggest you to test it before merging. The solution looks to simple for been error less.

Fix and simplify (DataSet) Next function
@sijms
Copy link
Owner

sijms commented Nov 27, 2020

there is a problem in this changes
you add line dataSet.index = 0 in command.Read() function
this function is called with fetch and calling fetch is depending on index value which must be correct until we
read all rows from the database

@sijms
Copy link
Owner

sijms commented Nov 27, 2020

actually I try the driver with many SELECT sql statement (return one row or return more than 1000 rows) and all rows are returned correctly same as sql developer

sorry if I am late in response because I am study the network behavior for LOB to add support for it

@simulot
Copy link
Contributor Author

simulot commented Nov 27, 2020

Take your time.

I have searched all code using dataSet.index, and it's used only in Next(). So I'm confident I can use it for tracking the row number in the current chunk instead of the absolute row in the dataset.

I'm doing systematic tests with a query that gives the exact number of rows we want:

select INVOICE_SUM A ,1 C  from DOCS where ROWNUM<=:1;

Current master c9e66ca works until 24 rows.
For 25 rows, it returns 0 row
For 26 rows, I get ORA-03120: two-task conversion routine: integer overflow

PR branch:
For 25 rows, it returns 25 rows
For 26 rows, I get ORA-03120: two-task conversion routine: integer overflow

If I start the test by requesting 26 rows, I don't get any error for both. Master branch is wrong, PR branch gives expected result

Provisional conclusion: PR behaves right up to 26 rows, but there is a problem with accumulation of queries.
I'm checking my code and driver's code.

With PR bran

I'll do more tests against a big table to compare extraction of a various number of lines and compare it. I'll check how the driver behaves around chunk limit of 25 rows.

@sijms
Copy link
Owner

sijms commented Nov 27, 2020

note
noOfRowsToFetch = 25
after you get 25 rows Next() will call fetch according to this code

if dataSet.parent.hasMoreRows && dataSet.index%dataSet.parent.noOfRowsToFetch == 0 {
	dataSet.Rows = make([]Row, 0, dataSet.parent.noOfRowsToFetch)
	err := dataSet.parent.fetch(dataSet)
	if err != nil {
		return err
	}
}

fetch will call Read() again
thus if read will change the value of index as you make:
dataSet.index = 0
the condition:
if dataSet.parent.hasMoreRows && dataSet.index%dataSet.parent.noOfRowsToFetch == 0
will give wrong result

@sijms
Copy link
Owner

sijms commented Nov 27, 2020

note that the operation is like fast read forward operation
when you read 2000 rows for example:
at any point of reading operation the driver memory will carry 25 rows only
thus at row no. 25 the Next() will call fetch to refill the internal array (which is 25 rows only) and will return the first row in the
array
25%noOfRowsToFetch = 0
the next row will be 26 which is 1 and so on until reach 50 and call fetch again and return row no 0 ... etc

@sijms
Copy link
Owner

sijms commented Nov 27, 2020

I find the problem
this is my mistake

I try to solve the issue #35
as database will return no record in first call of read and need to call fetch to get the first chunk

the original condition
if dataSet.parent.hasMoreRows && dataSet.index != 0 && dataSet.index%dataSet.parent.noOfRowsToFetch == 0 {

I change to
if dataSet.parent.hasMoreRows && dataSet.index%dataSet.parent.noOfRowsToFetch == 0 {

now I fall back to the original one to solve the problem and I will solve the problem of LONG and LOBs in another way

load last changes and It will work

@sijms sijms closed this Nov 27, 2020
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