In the BoF today at Drupalcon there seemed to be broad agreement that we should eliminate more of the non-dynamic fields in the schema since they can be confusing when trying to understand what and how to index.

CommentFileSizeAuthor
#14 1088208-giga-fix-14.patch17.58 KBpwolanin
#13 1088208-giga-fix-13.patch17.64 KBpwolanin
#12 1088208-giga-fix-12.patch17.1 KBpwolanin
#8 1088208-giga-fix.patch17.43 KBjpmckinney
#5 1088208-simplify-schema-5.patch27.76 KBpwolanin
#2 1088208-simplify-schema-2.patch14.83 KBpwolanin
#1 1088208-simplify-schema-1.patch905 bytespwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Active » Needs work
FileSize
1088208-simplify-schema-1.patch905 bytes

here's a start.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
1088208-simplify-schema-2.patch14.83 KB

Totally untested, but this removed a lot of the non-dynamic fields, and renames some of the others.

janusman’s picture

janusman CreditAttribution: janusman commented

Code looks good, let me actually apply it and do some testing. =)

janusman’s picture

janusman CreditAttribution: janusman commented

Just noticed we might have to update the tests too.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
1088208-simplify-schema-5.patch27.76 KB

committing this patch - might still have issues, but seems to basically work.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Fixed

Seems to basically be working - open new issues for major follow-up

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented

Just realized this breaks contrib modules like apachesolr_attachments... :(

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented
Status: Fixed » Needs work
FileSize
1088208-giga-fix.patch17.43 KB

Broken stuff:

* Can't sort by title
* apachesolr_delete_index can't delete by node type
* apachesolr_index_nodes doesn't report nids that failed indexing
* apachesolr_block_visibility doesn't respect node type restrictions
* apachesolr_mlt_suggestions doesn't respect node type restrictions
* excluding node types from index via admin doesn't delete them from index
* highlighting is broken
* drush solr-search doesn't report nid
* tests broken (maybe already broken)

These lines are mysterious to me:

$doc->created = strtotime($doc->ds_created);
$doc->changed = strtotime($doc->ds_changed);
$doc->name = $doc->tos_name;

Those fields are ignored by schema.xml. What use is it to assign them?

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented
Status: Needs work » Needs review
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

re: #8 - that's the processing of the response doc.

e.g. before the schema change it was:

$doc->changed = strtotime($doc->changed);
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

some mistaken conversions in there:

       foreach ($facet_info[$module][$delta]['content_types'] as $content_type) {
-        if ($query->has_filter('type', $content_type)) {
+        if ($query->has_filter('entity_type', $content_type)) {

should be:

       foreach ($facet_info[$module][$delta]['content_types'] as $content_type) {
-        if ($query->has_filter('type', $content_type)) {
+        if ($query->has_filter('bundle', $content_type)) {
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
1088208-giga-fix-12.patch17.1 KB

Most of the tests were totally non-functional other than the query class unit. tests. The changes here should not have been required to make that work, though useful to update the field names there (I knew they were out of date).

For date fields, it might be useful to have a more generic conversion to timestamps? e.g. 

foreach ($doc as $key => $value) {
  if (substr($key, 0, 3) == 'ds_') {
    $tskey = 'timestamp_' . substr($key, 2);
    $doc->$tskey = strtotime($value);
  }
}

rather than hard-coding the 2 used by nodes?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Fixed
FileSize
1088208-giga-fix-13.patch17.64 KB

Actually the drush command was totally broken - apachesolr_search_execute() doesn't exist in 7, it's apachesolr_search_search_execute()

Committing this. Re-open if we need any more cleanup.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
1088208-giga-fix-14.patch17.58 KB

oops - that messed up title in drush search. so actually committing this one.

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented

Cool, thanks for the review. I think that covers everything now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.