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

PR CONTRIB-6515 - Fixed bug with CSV export #73

Closed

Conversation

sarjona
Copy link

@sarjona sarjona commented Nov 8, 2016

Our databases are Oracle and we've detected this bug in some cases (with MySQL is working properly). We've tried to find a generic solution (in Oracle, converting TO_CHAR the qrX.response field is also working).

Tested in Moodle 3.1.2 Oracle databases.

This is an improving patch for the PR, which uses sql_order_by_text function instead of sql_compare_text: #68

For some DB, like Oracle, it's necessary to specify "The number of chars to use for this field" (for instance 1000), because CLOB fields don't support direct returning of such fields.

@sarjona sarjona mentioned this pull request Nov 8, 2016
@mchurchward
Copy link
Contributor

mchurchward commented Nov 8, 2016

Thanks for the new request. I'm sorry that this issue is taking so long, but I am struggling to understand the problem still.
The part you are fixing is the select field part of the SQL. You are changing the selection of "qro.response" to "$DB->sql_order_by_text('qro.response', 1000)".
For discussion, I will simplify the SQL.

In the current select statement, the raw SQL would look like this:

SELECT qro.response
FROM mdl_questionnaire_response_other qro;

Does that statement work in Oracle?

@timhunt
Copy link
Contributor

timhunt commented Nov 8, 2016

Yes, that statement works in Oracle.

The kind of thing that does not work is [WHERE text_column = varchar_column] or [ORDER BY text_column].

@mchurchward
Copy link
Contributor

Thanks Tim. Then I don't see how the proposed code change can fix the problem? It is adding the "sql_order_by_text" function to the field select portion of the SQL. And the "qro.response" field is not used in any comparison nor order statement. What am I not understanding?

@mchurchward
Copy link
Contributor

mchurchward commented Nov 8, 2016

Could the problem be caused by different types returned in the "response" column? For the sql, the response column can contain:

  • varchar(1),
  • longtext,
  • null.

Maybe we need to have a "cast" on all "response" returns to ensure that they are all the same?

On other words, something similar to what Dan proposed earlier:

if ($field === 'response') {
$extraselect .= "CAST($alias AS TEXT) AS $field";

@timhunt
Copy link
Contributor

timhunt commented Nov 8, 2016

Right, like with =, you can't mix these types in a UNION or similar.

@mchurchward
Copy link
Contributor

Okay. I now understand the need for this. Because we longtext and varchars being returned as UNIONS in the same field, Oracle needs to cast them to the same type.
So, using your latest "$DB->sql_order_by_text('qro.response', 1000)" makes sense.
Does the "1000" mean that it will only return up to 1000 characters? If so, that could still be a problem for essay question responses.

@mchurchward
Copy link
Contributor

@sarjona I added some comments and also modified the "boolean" type response here - b49abea
Can you let me know if this seems to work fine for your case?

@sarjona
Copy link
Author

sarjona commented Nov 11, 2016

@mchurchward Tested new version of the code with Oracle. We confirm everything is working perfectly!! :-) Thanks! :-)

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.

None yet

3 participants