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

Report limited indexes in MetaIndexes() #208

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ralfbecker
Copy link
Contributor

eg. "name(16)" instead of just "name", if only first 16 chars are indexed in column "name".

I need this information to create full backups including schema data.

As this might other use-cases of MataIndexes not expecting the limit reported this way, I could alternatively use an additional array-key instead of changing content returned in value of "columns".

Ralf

@ralfbecker
Copy link
Contributor Author

A more BC save approach would be to reported the limits of indexes under a different key, eg.

$link->MetaIndexes('my_table') = array(array(
    'unique' => false,
    'columns' => array('name'),
    'length' => array('name' => 16),
));

What do you think?

@dregad
Copy link
Member

dregad commented Feb 24, 2016

I think this is a better approach.

As a side note, I don't mind if you make changes to whitespace, but if you do please commit them separately separately from actual fixes, it makes code review easier. Thanks for your understanding.

@ralfbecker
Copy link
Contributor Author

No problem, should I fix that in this pull request and open an extra fix-trailing-whitespace pull request?

Anyway I'm happy to discuss and fix my pull requests one by one, so you can merge them, when you think they are ready.

@dregad
Copy link
Member

dregad commented Feb 24, 2016

should I fix that in this pull request and open an extra fix-trailing-whitespace pull request?

If you don't mind fixing it here, that would be great. It does not necessarily have to be a separate PR, but at least a distinct commit; note that the PR approach may create dependency issues / merge conflicts. Anyway, do whatever is more convenient for you.

@mnewnham
Copy link
Contributor

In general design terms, I would prefer that:

  1. the base level of the index definition not contain a non-portable attribute. (length supported in MySQL, Oracle but not SQL Server, Access etc)
  2. If we are going to provide access to a specific attribute of an index column (i.e. the indexed length of the column), we ought to be prepared to offer access to the other attributes, e.g. it's sort order (ASC,DESC).
  3. If we are prepared to offer access to the attributes of a column of an index, we ought be be prepared to offer the ability to access the attributes of an index itself.

So instead of:

'unique'=>0,
'columns'=>array(0=>'col1',
                1=>'col2',
                2=>col3),
'length'=>array('col2'=>16,
                 'col3'=>30)

We define it like this, with the column-attributes and index-attributes key always being defined, even as empty arrays.

'unique'=>0,
'columns'=>array(0=>'col1',
                1=>'col2',
                2=>'col3'),
'column-attributes'=>array('col2'=>array('length'=>16,
                                         'order'=>'desc'),
                           'col3'=>array('length'=>30),
'index-attributes'=array('type'=>'fulltext',
                            'comment'=>'some comment',
                           'algorithm'=>inplace')

The column and index attributes are then easily isolated on a data provider basis, and do not have to be 'portable' options Therefore we do not have to provide the ability to parse and understand the values in ADOdb, we can simply report them de-facto

Because the attributes are data-provider specific In this scenario, contributors can easily offer code snippets to identify provider specific index attributes, and we can add them without having to re-code the core methods each time

@dregad
Copy link
Member

dregad commented Sep 14, 2016

@ralfbecker what is the status on this ?

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