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

Add patch to fix test failures with SQlite 3.37.0 #142

Open
wants to merge 1 commit into
base: maint/0.0828xx
Choose a base branch
from

Conversation

Mic92
Copy link

@Mic92 Mic92 commented Jan 19, 2022

Alternative to #141

@Mic92
Copy link
Author

Mic92 commented Jan 19, 2022

cc @ribasushi

@ribasushi
Copy link
Collaborator

I am not sure this is the correct fix.

@charsbar what are you thoughts on this? Having DBD::SQLite return different column types depending on underlying libsqlite doesn't sound right. Are you amenable to lc()-ing them in DBD::SQLite directly? If no - I will lc() them on the DBIC side.

@charsbar
Copy link

@ribasushi
If the metadata has some significant role in DBIC, I'll try to tweak DBD::SQLite not to break compat. Otherwise, +1 for this PR, because...

  1. the value is directly set by sqlite3_table_column_metadata C API. DBD::SQLite (dbdimp.c) only newSVpv() it. https://github.com/DBD-SQLite/DBD-SQLite/blob/8bf916dbd7685dd9de7746015d2048d52a5ca7f8/dbdimp.c#L1955-L1970
  2. there seems to have long been a chance to return an upper case value. See the following excerpt from sqlite3.c in SQLite 3.9 (chosen randomly), especially "INTEGER" in the middle.
SQLITE_API int SQLITE_STDCALL sqlite3_table_column_metadata(
  sqlite3 *db,                /* Connection handle */
  const char *zDbName,        /* Database name or NULL */
  const char *zTableName,     /* Table name */
  const char *zColumnName,    /* Column name */
  char const **pzDataType,    /* OUTPUT: Declared data type */
  char const **pzCollSeq,     /* OUTPUT: Collation sequence name */
  int *pNotNull,              /* OUTPUT: True if NOT NULL constraint exists */
  int *pPrimaryKey,           /* OUTPUT: True if column part of PK */
  int *pAutoinc               /* OUTPUT: True if column is auto-increment */
){

(snip)

  /* The following block stores the meta information that will be returned
  ** to the caller in local variables zDataType, zCollSeq, notnull, primarykey
  ** and autoinc. At this point there are two possibilities:
  **
  **     1. The specified column name was rowid", "oid" or "_rowid_"
  **        and there is no explicitly declared IPK column.
  **
  **     2. The table is not a view and the column name identified an
  **        explicitly declared column. Copy meta information from *pCol.
  */
  if( pCol ){
    zDataType = pCol->zType;
    zCollSeq = pCol->zColl;
    notnull = pCol->notNull!=0;
    primarykey  = (pCol->colFlags & COLFLAG_PRIMKEY)!=0;
    autoinc = pTab->iPKey==iCol && (pTab->tabFlags & TF_Autoincrement)!=0;
  }else{
    zDataType = "INTEGER";
    primarykey = 1;
  }

(snip)

  /* Whether the function call succeeded or failed, set the output parameters
  ** to whatever their local counterparts contain. If an error did occur,
  ** this has the effect of zeroing all output parameters.
  */
  if( pzDataType ) *pzDataType = zDataType;
  if( pzCollSeq ) *pzCollSeq = zCollSeq;
  if( pNotNull ) *pNotNull = notnull;
  if( pPrimaryKey ) *pPrimaryKey = primarykey;
  if( pAutoinc ) *pAutoinc = autoinc;

(cut)

@ribasushi
Copy link
Collaborator

If the metadata has some significant role in DBIC

It has no significance to DBIC but it does have significance to users who might be doing if $column_info eq 'integer' or some case sensitive match, or who knows what ;)

I am definitely going to lc() this in software, and provide a consistent value in the colinfo structures. The only question ( and why I @ed you ) is: does this lc() live in DBIC or in DBD::SQLite. It's fine either way for me, but I need to know ;)

@charsbar
Copy link

does this lc() live in DBIC or in DBD::SQLite. It's fine either way for me, but I need to know ;)

Could you add it to DBIC? It should be easier in Perl.

@Mic92
Copy link
Author

Mic92 commented Jan 23, 2022

Sorry. I missing the context about the internals here to follow the discussion.

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.

4 participants